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

followup custom literals #17500

Merged
merged 8 commits into from
Mar 27, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Mar 25, 2021

compiler/ast.nim Outdated
@@ -1053,6 +1053,14 @@ const
defaultAlignment = -1
defaultOffset = -1

template getStrVal*(a: PNode): string =
## Returns underlying string for `{nkStrLit..nkTripleStrLit,nkSym,nkIdent}`.
let a2 = a
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't make it a template and use a directly.

Copy link
Member Author

@timotheecour timotheecour Mar 25, 2021

Choose a reason for hiding this comment

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

this avoids a copy; and lent is not possible.

EDIT: template doesn't actually avoid the copy in this case because of the case expression...

Copy link
Member Author

Choose a reason for hiding this comment

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

found a solution that avoids a copy (and uses a proc)

compiler/renderer.nim Outdated Show resolved Hide resolved
@timotheecour timotheecour force-pushed the pr_followup_customlit branch 2 times, most recently from e978564 to 853bbde Compare March 25, 2021 21:13
@timotheecour timotheecour marked this pull request as ready for review March 25, 2021 22:23
compiler/ast.nim Outdated
@@ -1054,6 +1054,20 @@ const
defaultOffset = -1


var emptyString = ""
Copy link
Member

@Araq Araq Mar 25, 2021

Choose a reason for hiding this comment

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

Come on... Instead try something like:

template strValProp*(a: PNode; prop: untyped): bool =
  case a.kind
  of nkStrLit..nkTripleStrLit: prop a.strVal
  of nkSym: prop a.sym.name.s
  of nkIdent: prop a.ident.s
  else: false

Copy link
Member Author

@timotheecour timotheecour Mar 26, 2021

Choose a reason for hiding this comment

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

I've tried (see option 2 and option 3, a variant of your proposal), it's objectively worse:

  • option 2: causes a copy, which is what i wanted to avoid in first place
  • option 3: causes code bloat, like multiple yield statements in inline iterators

and both options lead to more complex client code. How would you translate code like this with option 2 or 3?

echo (n[1].getStrVal, n[2].getStrVal, n.kind)

option 2: your suggestion

template strValProp*(a: PNode; prop: untyped): bool =
  let a2 = a # prevent double evaluation bugs
  case a2.kind
  of nkStrLit..nkTripleStrLit: prop a2.strVal
  of nkSym: prop a2.sym.name.s
  of nkIdent: prop a2.ident.s
  else: false
n[1].getStrVal.startsWith('\'')

gets replaced by:

from std/sugar import `=>`
n[1].strValProp((it: string) => it.startsWith('\''))

option 3: your suggestion modified

template strValIt*(a: PNode; op: untyped): bool =
  let a2 = a # prevent double evaluation bugs
  case a2.kind
  of nkStrLit..nkTripleStrLit:
    template it: untyped {.inject.} = a2.strVal
    op
  of nkSym:
    template it: untyped {.inject.} = a2.sym.name.s
    op
  of nkIdent:
    template it: untyped {.inject.} = a2.ident.s
    op
  else: false
n[1].getStrVal.startsWith('\'')

gets replaced by:

n[1].strValIt(it.startsWith('\''))

which isn't too bad in this case, but there's no simple equivalent in other cases, eg:

echo (n[1].getStrVal, n[2].getStrVal, n.kind)

@Araq
Copy link
Member

Araq commented Mar 27, 2021

So that we can proceed, please undo the changes to the following files for the time being:

  • compiler/ast.nim
  • compiler/msgs.nim
  • compiler/renderer.nim
  • tests/views/tcan_compile_nim.nim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants