cps, cps/environment: only desym environment fields #73
Conversation
This is a WIP commit. Experiments shows that we only have to desym due to issues in environment definition and access. Upstream bug: nim-lang/Nim#17851 This commit disable blanket desymification and only perform them on environment as a workaround for the mentioned bug, which fixes the foreign symbol tests.
alaviss
force-pushed
the
wip-selective-desym
branch
from
46b90e4 to
849d129
Compare
|
I'll need a bit of help to clean this up, the desym insertions is a hack atm, not to mention error prone. IMO we can build a kind of filter that desym any usage of a genSym-ed identifier from |
|
Gensym'd symbols have names that don't match their |
The better fix in nim-works#72 removed the need for this 🎉.
A proper workaround for nim-lang/Nim#17851, which lets us remove the desym injections into cps/environment.
|
It's working now, and it's beautiful. |
cps.nim
Outdated
| @@ -37,18 +38,19 @@ func getCallSym(n: NimNode): NimNode = | |||
| of nnkSym: | |||
| break | |||
| of nnkIdent: | |||
| raise newException(CatchableError, "This call is not typed") | |||
| result = nil | |||
| break | |||
| else: | |||
| assert false, "unknown node type: " & $result.kind | |||
There was a problem hiding this comment.
raise here? Or do you really want to skip this node in the event that assertions are disabled?
There was a problem hiding this comment.
Raising an error should be the job of whatever is calling this proc, imo. So the updated logic makes more sense.
There was a problem hiding this comment.
Yes, but then we assert false in the last leg...
There was a problem hiding this comment.
In fact, it's an infinite loop without assertions to crash it. Suggest you rewrite the case as if/if/else or whatever. All three of us missed it the first time around.
There was a problem hiding this comment.
Hmm, you're right that the logic is busted.
A break after the assert should be sufficient. During debug having it assert is handy for the smell factor until we narrow it down.
(Wow, typing this on the phone really messed it up)
There was a problem hiding this comment.
That sounds fine to me. 👍
cps/spec.nim
Outdated
| proc genField*(ident = ""): NimNode = | ||
| ## generate an unique field to put inside an object definition | ||
| ## | ||
| ## made as a walkaround for [nim-lang/Nim#17851](/~https://github.com/nim-lang/Nim/issues/17851) |
There was a problem hiding this comment.
s/walkaround/workaround/
cps.nim
Outdated
| @@ -37,18 +38,19 @@ func getCallSym(n: NimNode): NimNode = | |||
| of nnkSym: | |||
| break | |||
| of nnkIdent: | |||
| raise newException(CatchableError, "This call is not typed") | |||
| result = nil | |||
| break | |||
| else: | |||
| assert false, "unknown node type: " & $result.kind | |||
There was a problem hiding this comment.
Raising an error should be the job of whatever is calling this proc, imo. So the updated logic makes more sense.
alaviss
changed the title
Experiments shows that we only have to desym due to issues in
environment definition and access.
Upstream bug: nim-lang/Nim#17851
Has #72 bundled.
By keeping the symbols, we avoid the foreign bindsym issue altogether.