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

fix #4165(static parameters + emit) #17613

Closed
wants to merge 2 commits into from
Closed

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Apr 1, 2021

fix #4165

@ringabout ringabout changed the title fix #4165 fix #4165(static parameters + emit) Apr 2, 2021
@timotheecour
Copy link
Member

timotheecour commented Apr 2, 2021

this should be legal IMO, static should be no different from const here.

when true:
  proc printStr(s: static float) =
    const s2 = s
    # {.emit: """printf("%g\n", `s`);""" .} # bug but should be fixed instead of disallowing
    {.emit: """printf("%g\n", `s2`);""" .} # works
  printStr(1.23)

a fix similar to #17590 should be possible

@ringabout
Copy link
Member Author

ringabout commented Apr 2, 2021

It seems a bit different. Variable s disappears at runtime. But s2 still can be accessed at runtime.

@timotheecour
Copy link
Member

timotheecour commented Apr 2, 2021

the semantics should be:

  • a const gets inlined wherever it's being used
  • a static should act as if a const was defined with the static's value

Try this:

          if e.kind == skParam and e.typ != nil and e.typ.kind == tyStatic:
            result.add e.typ.n
          else:
            result.add newSymNode(e)
          incl(e.flags, sfUsed)

This seems to work, in at least the cases where a const would work eg:

when defined case5:
  proc printStr(s: static float) =
    const s2 = s
    {.emit: """printf("%g\n", `s`);""" .} # works
    {.emit: """printf("%g\n", `s2`);""" .} # works
  printStr(1.23)

as for the example in the bug report, it also wouldn't work with a const:

when true:
  proc printStr() =
    const s = "foo"
    # would not work, because a const gets inlined upon use
    {.emit: "puts(`s`->data);" .}
  printStr()

so that's a separate issue, and would be possible via timotheecour#553:
let a {.rom.} = constExpr

note that this works though, with my above patch:

when true:
  proc printStr(s: static string) =
    {.emit: """printf("%s\n", "`s`");""" .}
  printStr("abc")

EDIT

on 2nd though, the way you did it is probably simpler/more general; the error message could include something like suggesting to define a let s2 = s runtime variable before accesssing it

  • note that IMO the problem is identical for static and const, so whatever is done should apply to both

EDIT

but maybe it's still useful (eg performance reasons or simplicity) to support this use case:

when true:
  proc printStr(s: static string) =
    {.emit: """printf("%s\n", "`s`");""" .}
  printStr("abc")

which, ugly as it is, works in cases like this.

Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

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

LGTM

in future work we should also disable accessing a const inside an emit, for the same reasons.

instead of this:

when true:
  proc printStr(s: static string) =
    # now illegal for static after this PR, but not yet for const or generic static param
    {.emit: """printf("%s\n", "`s`");""" .}
  printStr("abc")

  proc printStr2[N: static string]() =
    # ditto: this still "works" after this PR but in future work should give CT error
    {.emit: """puts("`N`");""" .}
  printStr2["Hello, Nim"]()

users should turn the const or static into a codegen'd value via for eg:

when true:
  proc printStr(s: static string) =
    let s2 = s # now s2 is codegen'd
    {.emit: """printf("%s\n", `s2`->data);""" .}
    # or this:
    let s3 = s.cstring
    {.emit: """printf("%s\n", `s3`);""" .}
  printStr("abc")

@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Apr 3, 2021
@ringabout ringabout added the Ready For Review (please take another look): ready for next review round label Jun 23, 2021
@ringabout ringabout closed this Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round TODO: followup needed remove tag once fixed or tracked elsewhere
Projects
None yet
Development

Successfully merging this pull request may close these issues.

static[T] argument is not substituted properly in .emit pragma
2 participants