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

#9176 Fix inconsistent behavior of compile time macros when assigned to a constant #9517

Closed
wants to merge 3 commits into from

Conversation

ianmcxa
Copy link
Contributor

@ianmcxa ianmcxa commented Oct 26, 2018

Very simple change marking gorge, gorgeEx and staticExec as .compileTime. to fix inconsistent behavior when setting the result of gorgeEx to a constant.

@timotheecour
Copy link
Member

great!

  • do you know if there more cases like this?
    ie, what @LemonBoy described here:

Ehh, it's a stupid bug in the implementation of some compile-time only procs.

  • a test would be needed I think, eg maybe something like:
when defined(posix):
  const cmd = "echo foo" # or if you have another idea that makes it clear it's being run at CT, not at RT; but at least it'll catch the empty output bug of https://github.com/nim-lang/Nim/issues/9176
  var ret1 = staticExec(cmd)
  const tmp = staticExec(cmd)
  var ret2 = tmp
  doAssert ret1 == ret2

@ianmcxa
Copy link
Contributor Author

ianmcxa commented Oct 26, 2018

Should I make a test for each of the commands?

@timotheecour
Copy link
Member

meh, at least 1 is a good start

@krux02
Copy link
Contributor

krux02 commented Oct 26, 2018

Interesting diff, I like it. Please add a test case that fails on the old behavior and works correctly on the new behavior.

@ianmcxa
Copy link
Contributor Author

ianmcxa commented Oct 26, 2018

I've gone ahead and added a fix for getProjectPath and a test case to verify the fix in tests/system/compiletime.nim. I didn't see anything else that appeared to have this problem in vmopts.nim, but I am very new to the project so I may have missed something.

@timotheecour
Copy link
Member

timotheecour commented Oct 26, 2018

I've gone ahead and added a fix for getProjectPath

maybe update PR title

@ianmcxa ianmcxa changed the title #9176 Fix inconsistent behavior of gorgeEx #9176 Fix inconsistent behavior of compile time macros when assigned to a constant Oct 26, 2018
@mratsim
Copy link
Collaborator

mratsim commented Oct 27, 2018

Related: #8051

I've noticed that {.compileTime.} pragma doesn't work in many cases while static T return value works.

@dom96
Copy link
Contributor

dom96 commented Oct 27, 2018

This is great! But it has the disadvantage that it allows these compile-time-only procedures to be assigned in a runtime context, previously you would get a "Error: 'staticExec' can only be used in compile-time context" whereas with this PR you won't.

Unfortunately I think we need a different solution.

@Clyybber
Copy link
Contributor

@dom96 I think this is an acceptable tradeoff, because we have "static" in the name of the proc (ignoring gorge, since that alias is wierd anyways imo).

@ianmcxa
Copy link
Contributor Author

ianmcxa commented Oct 29, 2018

Why do we have gorge and gorgeEx anyway?

@skellock
Copy link
Contributor

gorgeEx gives you the exit code in addition to the stdout/stderr.

slurp and gorge are odd names for sure. I do prefer the much more boring static* nomenclature.

@ianmcxa
Copy link
Contributor Author

ianmcxa commented Oct 29, 2018

My question was just why do we have that naming convention in the first place? Why not a single proc that does what gorgeEx does now, but with a more descriptive name?

@skellock
Copy link
Contributor

Not sure of the heritage, but I like where you're going. #1994 is distantly related if we're talking about tidying up the API before 1.0.

@Araq
Copy link
Member

Araq commented Oct 30, 2018

Why do we have gorge and gorgeEx anyway?

Because some young fool (@Araq) had fun designing the language and added a single bad joke.

@@ -3789,12 +3789,10 @@ proc staticRead*(filename: string): string {.magic: "Slurp".}
##
## `slurp <#slurp>`_ is an alias for ``staticRead``.

proc gorge*(command: string, input = "", cache = ""): string {.
magic: "StaticExec".} = discard
Copy link
Member

Choose a reason for hiding this comment

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

Removing this magic here cannot be right.

## This is an alias for `staticExec <#staticExec>`_.

proc staticExec*(command: string, input = "", cache = ""): string {.
magic: "StaticExec".} = discard
Copy link
Member

Choose a reason for hiding this comment

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

Removing this magic here cannot be right.

@Araq
Copy link
Member

Araq commented Feb 23, 2019

This needs to be solved differently.

@Araq Araq closed this Feb 23, 2019
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.

8 participants