Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions compileopts/options.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,25 @@
package compileopts

import (
"fmt"
"strings"
)

var (
validGCOptions = []string{"none", "leaking", "extalloc", "conservative"}
validSchedulerOptions = []string{"tasks", "coroutines"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"none" is also a valid scheduler option

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaddr2line oh I see.. so I think I should also modify this line https://github.com/tinygo-org/tinygo/blob/master/main.go#L713 and this line: https://github.com/tinygo-org/tinygo/blob/master/compileopts/config.go#L132

can you please confirm?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes those should be updated. You could add it to this PR: even though it's somewhat separate it's a small and obvious fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaddr2line @aykevl done!

validPrintSizeOptions = []string{"none", "short", "full"}
// ErrGCInvalidOption is an error raised if gc option is not valid
ErrGCInvalidOption = fmt.Errorf(`invalid gc option: valid values are %s`,
strings.Join(validGCOptions, ", "))
// ErrSchedulerInvalidOption is an error raised if scheduler option is not valid
ErrSchedulerInvalidOption = fmt.Errorf(`invalid scheduler option: valid values are %s`,
strings.Join(validSchedulerOptions, ", "))
//ErrPrintSizeInvalidOption is an error raised if size option is not valid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need a space before the first word in a doc comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

ErrPrintSizeInvalidOption = fmt.Errorf(`invalid size option: valid values are %s`,
strings.Join(validPrintSizeOptions, ", "))
)

// Options contains extra options to give to the compiler. These options are
// usually passed from the command line.
type Options struct {
Expand All @@ -21,3 +41,42 @@ type Options struct {
TestConfig TestConfig
Programmer string
}

// Verify performs a validation on the given options, raising an error is options are not valid
// In particular:
// ErrGCInvalidOption will be reised if gc is not valid
// ErrSchedulerInvalidOption will be reised if scheduler is not valid
// ErrPrintSizeInvalidOption will be reised if size is not valid
func (o *Options) Verify() error {
if o.GC != "" {
valid := isInArray(validGCOptions, o.GC)
if !valid {
return ErrGCInvalidOption
}
}

if o.Scheduler != "" {
valid := isInArray(validSchedulerOptions, o.Scheduler)
if !valid {
return ErrSchedulerInvalidOption
}
}

if o.PrintSizes != "" {
valid := isInArray(validPrintSizeOptions, o.PrintSizes)
if !valid {
return ErrPrintSizeInvalidOption
}
}

return nil
}

func isInArray(arr []string, item string) bool {
for _, i := range arr {
if i == item {
return true
}
}
return false
}
122 changes: 122 additions & 0 deletions compileopts/options_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
package compileopts_test

import (
"testing"

"github.com/tinygo-org/tinygo/compileopts"
)

func TestOptions_Verify(t *testing.T) {
testCases := []struct {
name string
opts compileopts.Options
expectedError error
}{
{
name: "it returns no error if Options is empty",
opts: compileopts.Options{},
},
{
name: "it returns an error if gc option is not valid",
opts: compileopts.Options{
GC: "incorrect",
},
expectedError: compileopts.ErrGCInvalidOption,
},
{
name: "it returns no error if gc option is 'none'",
opts: compileopts.Options{
GC: "none",
},
},
{
name: "it returns no error if gc option is 'leaking'",
opts: compileopts.Options{
GC: "leaking",
},
},
{
name: "it returns no error if gc option is 'extalloc'",
opts: compileopts.Options{
GC: "extalloc",
},
},
{
name: "it returns no error if gc option is 'conservative'",
opts: compileopts.Options{
GC: "conservative",
},
},
{
name: "it returns no error if gc option is empty",
opts: compileopts.Options{
GC: "",
},
},
{
name: "it returns an error if scheduler option is not valid",
opts: compileopts.Options{
Scheduler: "incorrect",
},
expectedError: compileopts.ErrSchedulerInvalidOption,
},
{
name: "it returns no error if scheduler option is 'tasks'",
opts: compileopts.Options{
Scheduler: "tasks",
},
},
{
name: "it returns no error if scheduler option is 'coroutines'",
opts: compileopts.Options{
Scheduler: "coroutines",
},
},
{
name: "it returns no error if scheduler option is empty",
opts: compileopts.Options{
Scheduler: "",
},
},
{
name: "it returns an error if printSize option is not valid",
opts: compileopts.Options{
PrintSizes: "invalid",
},
expectedError: compileopts.ErrPrintSizeInvalidOption,
},
{
name: "it returns no error if printSize option is 'none'",
opts: compileopts.Options{
PrintSizes: "none",
},
},
{
name: "it returns no error if printSize option is 'short'",
opts: compileopts.Options{
PrintSizes: "short",
},
},
{
name: "it returns no error if printSize option is 'full'",
opts: compileopts.Options{
PrintSizes: "full",
},
},
{
name: "it returns no error if printSize option is empty",
opts: compileopts.Options{
PrintSizes: "",
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := tc.opts.Verify()
if tc.expectedError != err {
t.Errorf("expecting %v, got %v", tc.expectedError, err)
}
})
}
}
8 changes: 7 additions & 1 deletion main.go
Original file line number Diff line number Diff line change
Expand Up @@ -762,6 +762,13 @@ func main() {
Programmer: *programmer,
}

err := options.Verify()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether this is the right place to put this, as we continue modifying the options after this.

What do you think @aykevl ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this doubt as well, I put it here because it looked to me that after this line nothing was setting GC, Scheduler or PrintSize.. very curious to see what's you thoughts are..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it should be done when the options object is finished building and won't be modified anymore.

Looking down below I see that there is also a custom check for panicStrategy. I think it would make sense to move that one to the common Verify function too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aykevl I moved panicStrategy check under options as well, and removed it from main.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aykevl @jaddr2line I've also moved options.Validate call later in the code..
I'm still unsure about it, because a few lines later, in the "build" case of the switch, options.Target is modified.

Another option could be moving the option check into builder.NewConfig...
what do you guys think?

Copy link
Contributor Author

@cebernardi cebernardi May 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aykevl @jaddr2line what do you think about moving the options.Verify block into builder.NewConfig?
The more I think about it, the more it looks to me the right place 🤔

if err != nil {
fmt.Fprintln(os.Stderr, err.Error())
usage()
os.Exit(1)
}

if *cFlags != "" {
options.CFlags = strings.Split(*cFlags, " ")
}
Expand All @@ -776,7 +783,6 @@ func main() {
os.Exit(1)
}

var err error
if options.HeapSize, err = parseSize(*heapSize); err != nil {
fmt.Fprintln(os.Stderr, "Could not read heap size:", *heapSize)
usage()
Expand Down