-
-
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
fixes #10807 #10814
fixes #10807 #10814
Conversation
compiler/transf.nim
Outdated
@@ -762,6 +762,10 @@ proc transformCall(c: PTransf, n: PNode): PTransNode = | |||
inc(j) | |||
add(result, a.PTransNode) | |||
if len(result) == 2: result = result[1] | |||
elif magic == mAddr: | |||
result = newTransNode(nkHiddenAddr, n, 1) |
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.
Huh, how do you know it's nkHiddenAddr
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.
it is rather random choice, it can be nkAddr
if you like. I thought it is good idea to remove nkAddr
node in future and leave only call to mAddr magic
instead.
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.
Ah I see. But please make it an nkAddr
then.
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
should review for review once tests complete |
nim-lang/Nim#10807 changes received tree from nkAddr(subDocsId) to nkCall(addr(subDocsId)). Accept either old or new tree.
## Summary In typed AST, address-of operations are now *always* represented by `nkAddr` trees. This simplifies some compiler logic, makes processing for typed macros easier, and fixes an effect tracking bug with `addr`. ## Details This is effectively a revert of nim-lang/Nim#10814. Not turning calls to `mAddr` into `nkAddr` was done to prevent the unsafe address semantics from being lost, but those no longer exist. Lowering the call into an `nkAddr` tree has the benefit that it simplifies AST analysis and processing, as address-of operation can now always be detected by `PNode.kind == nkAddr`. Old code for detecting `mAddr` magic calls is removed. The effect tracking in `sempass2` had no special case for `mAddr` calls, thus treating them as normal calls, which led to `addr(x)` being treated as an indirect invocation of `x`, when `x` is of procedural type. With the `mAddr` call now being lowered earlier, this is no longer the case.
## Summary In typed AST, address-of operations are now *always* represented by `nkAddr` trees. This simplifies some compiler logic, makes processing for typed macros easier, and fixes an effect tracking bug with `addr`. ## Details This is effectively a revert of nim-lang/Nim#10814. Not turning calls to `mAddr` into `nkAddr` was done to prevent the unsafe address semantics from being lost, but those no longer exist. Lowering the call into an `nkAddr` tree has the benefit that it simplifies AST analysis and processing, as address-of operation can now always be detected by `PNode.kind == nkAddr`. Old code for detecting `mAddr` magic calls is removed. The effect tracking in `sempass2` had no special case for `mAddr` calls, thus treating them as normal calls, which led to `addr(x)` being treated as an indirect invocation of `x`, when `x` is of procedural type. With the `mAddr` call now being lowered earlier, this is no longer the case.
magic call to
mAddr
is now preserved till transformation stage where it is rewritten intonkHiddenAddr
.It is now possible to delete
nkAddr
node kind completely, let me know if you are interested in this kind of breaking change.