-
Notifications
You must be signed in to change notification settings - Fork 570
[go1.19] Fix build issue and add override signature directive #1260
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
40c5663
to
5f9eeb8
Compare
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.
Do we need a separate pragma for overriding signatures, given that we already have the ability to override whole functions? It seems like it would only be useful in a very narrow set of cases.
Also, could you please add all newly added directives to doc/pragma.md? I forgot to ask about that in the earlier PR, sorry.
@nevkontakte TBH we probably don't need to have The main reason I added it was for some methods in crypto that have type parameters (e.g. |
@nevkontakte I also added to |
Thanks for the explanation! I was not opposed to addition of the directive per se, but I wanted to make sure I fully understand the logic behind it instead of making my own assumptions :)
That's actually a strong argument. Messing with cryptography makes bad things happen 🙃
That makes the two of us, as you may have notices from typos in comments 😄 No worries! If I spot anything, I'll let you know. |
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.
Thank you for the contribution!
The build updates changes I did will still remove imports which are needed by directive only (i.e.
unsafe
forgo:linkname
andembed
forgo:embed
). These directives are also read from the top-level comments by the compiler so we may not simply clear out the top-level comments.The import remover also had to deal with files like src/math/rand/race_test.go which has a dot import (
import . "math/rand"
). Typically we leave dot imports alone because it is hard to determine if they are used or not, however in this case, the test is removed making the import unused. So now, if the file is empty of anything but imports or thego:linkname
directive (we don't have to worry about theembed
since that needs a target variable), clear out everything including dot and blank (_
) imports.Add
//gopherjs:override-signature
directive which allows us to replace a function signature with another while not modifying the body of the function. The two functions will still have to matchFuncKey
which is based off the receiver and function name.