Nim icon indicating copy to clipboard operation
Nim copied to clipboard

store full definition AST for consts, fix noRewrite

Open metagn opened this issue 1 year ago • 5 comments

closes #9331, fixes #20114, fixes #19766

Stores full definition AST for consts (continuing #9582). Also moves extractPragma to ast for later convenience. Also had to fix noRewrite (#16620, probably not fully ready to close yet) and use it because storing var pragma AST somehow unlocked a broken term rewriting macro in a test. Genuinely no idea how, don't care to investigate.

Unsure about compile option like #9582 or something like nimHasConstDefImpl. Users should check for nnkConstDef anyway since sometimes it's still not ConstDef.

metagn avatar Jul 31 '22 06:07 metagn

We should instead or in addtion to that refactor the compiler transformation pipelines.

Araq avatar Sep 03 '22 08:09 Araq

Is there any way of checking if this is backportable? It would help to be able to see {.strdefine.} in docs for consts on stable versions, but I'm unsure if the patch is compatible.

metagn avatar Sep 13 '22 04:09 metagn

I now believe this should be backported, there might be minor conflicts but I can help resolve them if needed.

metagn avatar Sep 17 '22 06:09 metagn

While the PR itself is fine, this seems way too risky for a backport. What do you think?

Araq avatar Sep 21 '22 08:09 Araq

I can just make another PR for the 1.6 branch, so I'll unmark as backport.

As for the PR content, on second thought it is possible this will break code, so by principle it shouldn't be backported. But it can very well be considered a bugfix.

So no backport probably.

metagn avatar Sep 21 '22 09:09 metagn

Can someone point me to why CI is failing after the rebase but it wasn't a couple days ago? I'm guessing it has to do with https://github.com/nim-lang/Nim/commit/7739e23420c9a7a4ec7eccdddc0a39df9e905aa8 but I have no idea how the error messages relate to this PR specifically.

Edit: Never mind, I think I found it

metagn avatar Sep 24 '22 11:09 metagn

At this point I don't think this can be backported as-is anyway. So I don't think there's anything left to do.

metagn avatar Sep 28 '22 10:09 metagn

Thanks for your hard work on this PR! The lines below are statistics of the Nim compiler built from de4b0346bdafab6c38b77d430d0e83f95da0582c

Hint: mm: orc; threads: on; opt: speed; options: -d:release 164246 lines; 11.828s; 667.68MiB peakmem

github-actions[bot] avatar Sep 28 '22 13:09 github-actions[bot]