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

Get symbol kind #7491

Merged
merged 12 commits into from
Apr 11, 2018
Merged

Get symbol kind #7491

merged 12 commits into from
Apr 11, 2018

Conversation

krux02
Copy link
Contributor

@krux02 krux02 commented Apr 3, 2018

fixes #7366

when n is of type NimNode, then you can do the following:

let symKind = n.symbol.kind

@krux02 krux02 force-pushed the get-symbol-kind branch from 55259b1 to 955f6f3 Compare April 3, 2018 23:35
@krux02
Copy link
Contributor Author

krux02 commented Apr 4, 2018

Something I would like to discuss here is the usefulness of the NimSym type itself. The only functions that accept a NimSym as argument, are the following:

proc symbol=(n: NimNode; val: NimSym)
proc `$`(s: NimSym): string
proc `==`(a, b: NimSym): bool
proc getImpl(s: NimSym): NimNode
# and with this PR also this one
proc kind(s: NimSym): NimSymKind

getImpl and kind are the only "useful" functions here, and except from the getter symbol there is no way to get a NimSym. Both genSymand bindSym return a NimNode not a NimSym. both getImpl and kind could easily be replaced by functions that also accept a NimNode as argument and expect the node to be of kind nnkSym. Of coure would it be required to be rename kind to something else like symKind or symbolKind.

let impl = n.symbol.getImpl
let kind = n.symbol.kind
# would become
let impl = n.getImpl
let kind = n.symKind

@Araq
Copy link
Member

Araq commented Apr 4, 2018

Something I would like to discuss here is the usefulness of the NimSym type itself.

Yeah, we should do without, I agree, good points!

@krux02
Copy link
Contributor Author

krux02 commented Apr 4, 2018

Should I implement getImpl and symKind as proposed and tag following functions as deprecated, in this PR?

proc symbol(n: NimNode): NimSym
proc symbol=(n: NimNode; val: NimSym)
proc `$`(s: NimSym): string
proc `==`(a, b: NimSym): bool
proc getImpl(s: NimSym): NimNode

@Araq
Copy link
Member

Araq commented Apr 4, 2018

Yes.

@krux02 krux02 force-pushed the get-symbol-kind branch from 6349c50 to 3786443 Compare April 5, 2018 17:30
@krux02
Copy link
Contributor Author

krux02 commented Apr 5, 2018

There is one thing I would like to discuss here, too. The NimIdent
type is already almost obsolete. These are the functions that are all
methods that take a NimIdent

# NimIdent result
proc toNimIdent*(s: string): NimIdent
proc ident*(n: NimNode): NimIdent

# NimIdent argument
proc `!`*(s: string): NimIdent
proc `$`*(i: NimIdent): string
proc `==`*(a, b: NimIdent): bool
proc `ident=`*(n: NimNode, val: NimIdent)
proc newIdentNode*(i: NimIdent): NimNode
proc newCall*(theProc: NimIdent, args: varargs[NimNode]): NimNode
proc nestList*(theProc: NimIdent, x: NimNode): NimNode

The only function that doesn't work with a NimNode is nestList. And
that one can easily be ported.
It is just proc `$`(n: NimNode) that relies on
proc `$`(i: NimIdent) for it's implementation. But apart from
that, NimIdent is ready for deprecation, too.

I think the best road to deprecate NimIdent is to rename symName that I just implemented to just name, and make it work on both identifiers and symbols.

Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Just some small comments. Looks good.

proc ident*(n: NimNode): NimIdent {.magic: "NIdent", noSideEffect.}

proc symbol*(n: NimNode): NimSym {.magic: "NSymbol", noSideEffect, deprecated.}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a doc comment with the usual deprecation template.

proc symName*(n: NimNode): string =
$n.symbol
proc getImpl*(symbol: NimNode): NimNode =
symbol.symbol.getImpl
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these definitions really necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes they are, otherwise bootstrapping breaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well at least when the bootstrapping compiler is updated, the when statement with the else branch can completely disappear.

@Araq
Copy link
Member

Araq commented Apr 6, 2018

Looks excellent, needs a changelog entry and more documentation overall. Also check if the existing documentation is outdated with this change.

@krux02
Copy link
Contributor Author

krux02 commented Apr 11, 2018

using strVal= for nkIdent nkSym does not work. For consistency I could make it work, but I don't think they should be used to set the string in either symbols not identifiers. Apart from that, this PR is ready for another review. A lot of changes went into this now.

@Araq
Copy link
Member

Araq commented Apr 11, 2018

It's really nice. I have nothing to complain. Not merging though since you had some questions in IRC.

@Araq Araq merged commit 6baca58 into nim-lang:devel Apr 11, 2018
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.

Feature request - macros: Get the NimSymKind of a NimSym
3 participants