-
-
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
fix #14339, #13511, #14420: fixes limited VM support for addr #16002
Conversation
1231b1f
to
8bba52a
Compare
19cf700
to
f3afd95
Compare
954e7b2
to
c93ba02
Compare
regs[ra].nodeAddr = addr(regs[rb].node) | ||
of rkNodeAddr: # bug #14339 | ||
regs[ra].nodeAddr = regs[rb].nodeAddr |
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.
Doesn't that mean that when you do somePtr = addr someOtherPtr
it will behave like somePtr = someOtherPtr
?
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 don't believe so; eg:
when true:
type Foo = ref object
f0: int
proc main=
var f = Foo(f0: 3)
var f2 = f.addr
var f3 = f2.addr
var f4 = f3
f4.f0 += 1 # like f3.f0
f4[].f0 += 1 # like f2.f0
f4[][].f0 += 1 # like f.f0
f4[][][].f0 += 1 # like f[].f0
doAssert f[].f0 == 7
static: main()
main()
=> CT behaves like RT
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.
Hmm, I'm struggling to come up with a concrete example, but please explain why it is correct (?) and maybe add a comment.
Your example isn't exactly convincing because f4
is ptr ptr Foo
and it gets automatically dereferenced. If you change Foo
to be a non-ref object it fails.
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.
If you change Foo to be a non-ref object it fails.
just filed #16020, but this is a pre-existing bug and this PR exercises a different code path, see https://gist.github.com/timotheecour/fc575c054cddba5bea32df98d1c50cdc
Hmm, I'm struggling to come up with a concrete example, but please explain why it is correct (?) and maybe add a comment.
TODO on me
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.
@Clyybber my understanding is there's an implicit deref that occurs in code paths that lead here due to semantic transform that generates a temporary for block return expressions (without result = expr
or return expr`)
eg:
when true: # D20201125T111240
template main() =
type
Node = ref object
val: int
proc bar(c: Node): var int =
discard
c.val # before PR: Error: limited VM support for 'addr' (but nim was pointing at wrong lineinfo)
var a = Node()
discard a.bar()
static: main()
which is transformed into:
proc bar(c: Node): var int =
result = block:
discard
c.val
instead of:
proc bar(c: Node): var int =
discard
result = c.val
this seems related to #16137
This PR is still good IMO (more things now work, and hopefully nothing breaks), but I agree that the code transformation needs to be revisited as @cooldome mentioned here: #14420 (comment)
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 code transformation is correct and is not what my question is about.
I'm asking how treating ... = addr someAddress
as if it were ... = someAddress
is correct/sound here.
d90f766
to
a7bb1f2
Compare
…support for addr (nim-lang#16002) * fix nim-lang#14339: fixes limited VM support for addr * strengthen test * reference bug nim-lang#16003 * also fixes nim-lang#13511 * also fixes nim-lang#14420
…support for addr (nim-lang#16002) * fix nim-lang#14339: fixes limited VM support for addr * strengthen test * reference bug nim-lang#16003 * also fixes nim-lang#13511 * also fixes nim-lang#14420
result
for lent or var annotations to work #14420 (all snippets are tested in this PR, with c,cpp,js,vm)future work
now that #14420 will be closed (all snippets work in all backends), decide whether we still need to do anything about #14420 (comment), which is a separate issue /cc @cooldome