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

correct macros.genSym comment doc #20633

Closed
wants to merge 1 commit into from

Conversation

bung87
Copy link
Collaborator

@bung87 bung87 commented Oct 23, 2022

refs: #17851

## Generates a fresh symbol that is guaranteed to be unique. The symbol
## needs to occur in a declaration context.
## Generates a fresh symbol.
## if `ident` is not empty, the generated symbol maybe ambiguous.
Copy link
Member

@Araq Araq Oct 24, 2022

Choose a reason for hiding this comment

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

No, it's really always unique, it's just that the string representation of the code might not reflect this:

    of opcGenSym:
      decodeBC(rkNode)
      let k = regs[rb].intVal
      let name = if regs[rc].node.strVal.len == 0: ":tmp"
                 else: regs[rc].node.strVal
      if k < 0 or k > ord(high(TSymKind)):
        internalError(c.config, c.debug[pc], "request to create symbol of invalid kind")
      var sym = newSym(k.TSymKind, getIdent(c.cache, name), nextSymId c.idgen, c.module.owner, c.debug[pc])
      incl(sym.flags, sfGenSym)
      regs[ra].node = newSymNode(sym)
      regs[ra].node.flags.incl nfIsRef

Copy link
Collaborator Author

@bung87 bung87 Oct 24, 2022

Choose a reason for hiding this comment

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

the problem is the symbol id is unique but the ident id is not. which will cause ambiguous, I've seen serveral issue about macro users submited issues about this.

Copy link
Member

Choose a reason for hiding this comment

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

So users need to understand symbols but your documentation patch makes it worse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay then, someone may have good one in mind.

@Varriount Varriount added the Requires Araq To Merge PR should only be merged by Araq label Oct 24, 2022
@Araq Araq closed this Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Requires Araq To Merge PR should only be merged by Araq
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants