Skip to content
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

[backport] fixes #23711; C code contains backtick`gensym #23716

Merged
merged 3 commits into from
Jun 19, 2024
Merged

Conversation

ringabout
Copy link
Member

fixes #23711

@@ -555,11 +555,15 @@ proc semIdentWithPragma(c: PContext, kind: TSymKind, n: PNode,
else: discard
else:
result = semIdentVis(c, kind, n, allowed)
let invalidPragmasForPush = if fromTopLevel and "`gensym" notin result.name.s:
Copy link
Collaborator

@metagn metagn Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we maybe remove the check for `gensym here and mangle the output of {.exportc.} when no string is given to it instead? Because this errors too despite not being skVar/skLet/skConst (#20911):

{.push exportc.}

template foo =
  proc bar {.gensym.} = discard

foo()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too hacky.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too hacky.

How should we check gensym? There is not a flag dedicated for it since it is a newly created ident.

if sfGenSym in s.flags:
  result.add newIdentNode(getIdent(c.ic, x.name.s & "`gensym" & $c.instID),
    if c.instLines: actual.info else: templ.info)

mangle the output of {.exportc.} when no string is given to it instead

I would like to revive #19636 with some adaptations

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduce a new flag like sfWasGenSym.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I will move `gensym check into newSymG and incl the flag there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least make the check cheaper by doing:

getIdent(c.ic, "`" & x.name.s & "`gensym" & $c.instID)
...

s.name.s[0] == '`'

Copy link
Member Author

@ringabout ringabout Jun 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if the name is changed, it is going to break https://github.com/iffy/nim-argparse/blob/4048a76b24919807b0871dd145ba93a48c2688e7/src/argparse/macrohelp.nim#L36 which splits gensym based on "`". Since we are fixing a regression, I will settle for a comprised solution:

if find(result.name.s, '`') >= 0:
  result.flags.incl sfWasGenSym

@ringabout
Copy link
Member Author

dnsclient in important packages seems to be quite unstable or broken

@Araq Araq merged commit 646bd99 into devel Jun 19, 2024
22 checks passed
@Araq Araq deleted the pr_based_foo branch June 19, 2024 06:33
Copy link
Contributor

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

Hint: mm: orc; opt: speed; options: -d:release
179014 lines; 8.461s; 664.309MiB peakmem

narimiran pushed a commit that referenced this pull request Jun 24, 2024
Araq pushed a commit that referenced this pull request Jul 17, 2024
This adds Constantine to the important packages. Release announcements:
- https://forum.nim-lang.org/t/11935
- https://github.com/mratsim/constantine/releases/tag/v0.1.0

Unfortunately at the moment I'm in a conundrum.

- Constantine cannot compile on devel due to
#23547
- The workaround is changing 
  ```Nim
func mulCheckSparse*(a: var QuadraticExt, b: static QuadraticExt)
{.inline.} =
  ```
  to
  ```Nim
  template mulCheckSparse*(a: var QuadraticExt, b: QuadraticExt) =
  ```
but this does not compile on v2.0.8 due to `gensym` issues despite
#23716

![image](https://github.com/nim-lang/Nim/assets/22738317/21c875d7-512f-4c21-8547-d12534e93a58).
i.e. as mentioned in the issue
#23711 there is another gensym bug
within templates that was fixed in devel but not the v2.0.x series and
that is not fixed by #23716
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Semi-regression] C code contains backtick`gensym
3 participants