-
Notifications
You must be signed in to change notification settings - Fork 38
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
macro pragmas: support template and const defs #388
macro pragmas: support template and const defs #388
Conversation
db444ce
to
0a4dbe7
Compare
bors r+ |
388: macro pragmas: support template and const defs r=saem a=saem ## Summary: * constdef annotation handling same as identdefs in let or var * annotation processing on template definitions like other routine kinds ## Details: * reenabled `tests/pragmas/tpragmas_misc` * updated manual for macros pragmas * `semstmts.semConst` reworked to be like `semstmts.semLetOrVar` * constdef annotation handling same as identdefs in let or var * `semLetOrVarAnnotation` works on const defs, and renamed * added tracing to `semTemplateDef` * added tests for let/var section processing Mostly a port of nim-lang/Nim#19406 Co-authored-by: saem <saemghani+github@gmail.com>
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 left a few comments regarding allowed pragmas on consts
and also a few suggestions for documentation improvements, but overall, it looks really nice.
if def.kind != nkNilLit: | ||
# xxx: this seems wrong, instead of guarding on nil, maybe it should be | ||
# inferred from `typ`? then we still do the rest of the checks? |
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 nkNilLit
check here is done in order to support nil ref
, ptr
and pointer
values for const
s (they would be rejected by typeAllowed
otherwise).
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.
Yeah, but that sounds like a bug with typeAllowed
that we're working around here?
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.
Well, typeAllowed
only looks at the type, so I wouldn't consider it a bug.
Thinking about this a bit more, we don't want to disallow ref
and ptr
in general, but only non-nil
values of these types. With the current implementation,
type A = object
x: ref int
const c = A()
is rejected, even though it'd work fine.
Rejecting non-nil
values for ptr
, ref
, and closure environments is actually already implemented (the rsemVmUnsupportedNonNil
report; see deserializeRef
and deserialize
in vmcompilerserdes.nim
), so the nkNilLit
guard here could be removed (after adjusting typeAllowed
).
typeAllowed
is still needed here, in order to reject objects using inheritance (the back-ends don't support those yet if I remember correctly).
I do think it's better to do this in a separate PR however.
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.
Totally agree on the separate PR.
As for whether it's a bug or not. typeAllowed
does know it's a const/let/var. It's also only receiving typ
, and not def
. So the only pathway for def
to influence typeAllowed
is the earlier derivation of typ
from def
. In which case, shouldn't typ
be correctly constructed such that typeAllowed
doesn't reject it?
Correctly constructed meaning, given:
const foo: SomeRefType = nil
# or
const foo: SomeRefType = evalsToNil()
even if def
is a nil it should make no difference as typ
should be correctly derived. Unless, typ
is not being derived correctly from def
and/or we're missing a user specified type expession and we have a pure nil value (kinda like a void pointer).
Maybe I'm totally misunderstanding it?
bors cancel |
Canceled. |
46cbf13
to
45ce390
Compare
Squashed with all the latest. |
bors r+ |
👎 Rejected by PR status |
bors r+ |
👎 Rejected by PR status |
* constdef annotation handling same as identdefs in let or var * annotation processing on template definitions like other routine kinds Details: * reenabled `tests/pragmas/tpragmas_misc` * updated manual for macros pragmas * `semstmts.semConst` reworked to be like `semstmts.semLetOrVar` * constdef annotation handling same as identdefs in let or var * `semLetOrVarAnnotation` works on const defs, and renamed * added tracing to `semTemplateDef` * added tests for let/var section processing Mostly a port of nim-lang/Nim#19406
45ce390
to
a0f8e2a
Compare
Not sure what exactly broke reproducible builds, have to figure out how to figure that out. 😓 But in the meantime rebased on the latest devel as it contains some workflow updates and perhaps that's might address this. 🤞🏽 |
bors r+ |
Build succeeded: |
Summary:
Details:
tests/pragmas/tpragmas_misc
semstmts.semConst
reworked to be likesemstmts.semLetOrVar
semLetOrVarAnnotation
works on const defs, and renamedsemTemplateDef
Mostly a port of nim-lang/Nim#19406