-
Notifications
You must be signed in to change notification settings - Fork 570
compiler: fix variadic args not being nil when zero length. #807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
tests/compiler_test.go
Outdated
if v := printVari(); v != nil { | ||
t.Errorf("expected printVari() to be %v; got: %v", nil, v) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't tests/misc_test.go
a proper place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests
package needs a bit of a cleanup at some point. The segmentation is, I agree, not great. I even be tempted to have subpackages to better segment things.
But that belongs in a later PR. Not critical either way in my opinion whether this goes in misc_test.go
or a new file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering why compiler_test.go
didn't exist until this PR.
I tried this out and everything looks good on my end. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments about the test. Aside from that, not seeing issues.
tests/compiler_test.go
Outdated
@@ -0,0 +1,17 @@ | |||
// +build js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use t.Skip
rather than build constraints for skipping tests meant for GOARCH=js
only, unless the build constraint is necessary for things to compile.
In this case, this test shouldn't be skipped on non-js
; I want to see it run and pass using gc
as well (just like all other tests in this package that which can pass with gc
).
So, remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
tests/compiler_test.go
Outdated
return strs | ||
} | ||
|
||
if v := printVari(); v != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to be more consistent and stick with the got, want
test style:
if got := printVari(); got != nil {
t.Errorf("printVari() returned: %#v, want: %#v", got, []string(nil))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm intentionally using untyped nil
here; but I've renamed the got
variable.
Thanks @shurcooL. I've pushed up a commit that addresses your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
tests/compiler_test.go
Outdated
} | ||
|
||
if got := printVari(); got != nil { | ||
t.Errorf("expected printVari() to be %v; got: %v", nil, got) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want us to be more consistent with the style used in most Go tests, which is to print the "got" value first, the "want" value second. Mind changing it?
t.Errorf("printVari() returned: %#v, want: %#v", got, []string(nil))
Using %#v
instead of %v
has the advantage that it actually differentiates nil slice vs non-nil empty slice. %v
prints both as []
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Just pushed up a commit that does this.
Please can I ask in return that you start a wiki, that we can then build on, which documents the conventions you would like to follow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. But why did you replace []string(nil)
with just nil
? It's not the same type. Not critical, just pointing out.
Please can I ask in return that you start a wiki, that we can then build on, which documents the conventions you would like to follow?
We already have exactly that. Please see https://github.com/gopherjs/gopherjs/wiki/Developer-Guidelines#style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Actually, https://play.golang.org/p/L31k_UZViaQ is a better example of what I was pointing out.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why did you replace []string(nil) with just nil? It's not the same type. Not critical, just pointing out.
Yes, I know it's not the same type. But as I said in the comment above (that you 👍 -ed) it is an intentional use of nil
vs []string(nil)
, precisely because the GopherJS compiler will see these as different types. We want to ensure that the comparison with untyped nil works (we could also add a test for typed nil but that doesn't seem particularly important).
We already have exactly that. Please see https://github.com/gopherjs/gopherjs/wiki/Developer-Guidelines#style.
Great, thanks - I'd clearly missed this before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know it's not the same type. But as I said in the comment above (that you 👍 -ed) it is an intentional use of
nil
vs[]string(nil)
, precisely because the GopherJS compiler will see these as different types.
I thought you were referring to using nil
in the if
statement condition. That's what my 👍 reaction was referring to. Using an untyped nil
there makes sense, since if got != []byte(nil)
is (sadly) not allowed in Go (i.e., it'd be a compilation error).
We want to ensure that the comparison with untyped nil works (we could also add a test for typed nil but that doesn't seem particularly important).
My comments aren't about the comparison (keeping it as is is fine), they're about the t.Errorf
error message. I was hoping the https://play.golang.org/p/L31k_UZViaQ snippet would explain why my suggestion for changing the t.Errorf
message would make it slightly more clear.
This is such a trivial thing, I don't mind the existing error message either (that's why this PR has my LGTM since earlier). I just wanted to clarify what I meant.
Great, thanks - I'd clearly missed this before.
No problem.
I think the Go and Upspin projects serve as really good references for when there's uncertainty about how to best do something. The way I try to think about it is this: if it were a CL sent to the Go project and someone like Brad Fitzpatrick or Ian Lance Taylor saw it, would they be happy to approve it? It's a really good practice to look over the CLs to the Go project and learn from the code and reviews there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure the test rule, but I'm fine as @shurcooL is fine
tests/compiler_test.go
Outdated
if v := printVari(); v != nil { | ||
t.Errorf("expected printVari() to be %v; got: %v", nil, v) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering why compiler_test.go
didn't exist until this PR.
@hajimehoshi indeed, |
Yeah, I don't mean we should not have |
I wonder if we handle the case where an empty but non-nil slice is passed to a variadic function via |
I tested it, and that case is handled correctly. But, I think it'd be great to add a test case for it (as far as I can see, we don't have one). Mind adding one @myitcv? (Otherwise, I can add it before merging.) |
Merge? |
@myitcv I'm finally getting through the backlog of issues and pull requests. Could you rebase this PR on top of the latest master so that we can re-run the current test suite? Sorry it took this long... |
I've rebased this here: https://github.com/gopherjs/gopherjs/tree/fix_806 and opened a new PR #1096 to replace this one. |
Fixes #806.