-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
custom pragmas: correct error condition, remove outdated symkind whitelist #21653
Conversation
elif sym != nil: | ||
illegalCustomPragma(c, it, sym) | ||
else: | ||
invalidPragma(c, it) |
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.
This branch was also never called as sym != nil
is guaranteed to be true
So this doesn't change general performance either
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
…elist (nim-lang#21653) * test not restricting custom pragma applied symbols fixes nim-lang#21652 * fix other test * different patch * fix tests * actually test nim-lang#18212 and other routines
…elist (nim-lang#21653) * test not restricting custom pragma applied symbols fixes nim-lang#21652 * fix other test * different patch * fix tests * actually test nim-lang#18212 and other routines
…elist (nim-lang#21653) * test not restricting custom pragma applied symbols fixes nim-lang#21652 * fix other test * different patch * fix tests * actually test nim-lang#18212 and other routines
fixes #21652
Along with fixing the error message, instead of using a whitelist of symbol kinds for custom pragmas, use a blacklist of ones that do not work, so we don't have to keep maintaining this list as much (a symbol kind that does not support custom pragmas silently accepts it in the worst case). Currently this is enum fields,
for
loop variables (both because they do not retain pragma AST after semcheck) and module symbols (as it's not handled). We can also whitelistPragmaApplicableSymKinds - {skEnumField, skForVar, skModule}
instead to make it more restrictive, but again in the worst case with the blacklist it is just silently accepted.Description for previous patch in this PR
The symbol kinds you could previously not apply custom pragmas to were:
Now trying to apply a nonexistant pragma to these symbols will give a proper "invalid pragma" error instead of the "cannot attach custom pragma" error.
This way of doing it will attempt to "attach" existing custom pragmas to these symbol kinds, which should not be a problem for macros and templates, but just swallow the custom pragmas without erroring likely as semchecking removes the pragma AST down the line.
We don't have to fix #21652 this way, there is an easy patch of delaying the "cannot attach custom pragma" error until we succeed in finding the custom pragma in
semCustomPragma
, but I want to test this "swallowing" behavior against CI first to see if it breaks anything.