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

add compilesettings.nimVersionCT + friends #14648

Closed
wants to merge 1 commit into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jun 13, 2020

  • refs make proc nimQuery(setting: Setting): RootObj {.magic.} => return a type that depends on the enum compilerdev#9
  • after this PR compiler/condsyms should rarely be needed since you can just do a version test (which self-document when something was added as side benefit)
  • since is a different thing since it looks at system.NimMajor+friends, which says nothing about the stdlib version nim was compiled at, so is suitable when branching for stdlib features, but not compiler features
  • also added nimVersion for symmetry, which is useful when since falls short, eg: when nimVersion >= (1,3,5): foo else: bar, or when defined(js) and nimVersion >= (1,3,5): bar etc
  • no system bloat

example

# in ~/.config/nim/config.nims
import std/compilesettings
when nimVersionCT >= (1,3,5):
  # instead of having to add `defineSymbol("nimHasStacktraceMsgs")` to condsyms.nim
  switch("stacktraceMsgs", "on") # or any thing that is defined in compiler (eg magics) as opposed to stdlib

after that I'll resume #14068 (comment)

links

(EDIT)
https://github.com/nim-lang/Nim/pull/16406/files/e1b4468313399c0468c6675bf18a62c7e15b233c..f8d5208f2e377e2bd7c52ed8a96f18255c256850#r549607059

@dom96
Copy link
Contributor

dom96 commented Jun 13, 2020

Why do you need this?

@timotheecour
Copy link
Member Author

timotheecour commented Jun 13, 2020

Why do you need this?

to allow branching on compiler version (the (NimMajor,NimMinor,NimPatch) recorded when it was compiled) without shelling out via nim dump --config:json

branching directly on (NimMajor,NimMinor,NimPatch) is the wrong thing when the feature is defined (or bug is fixed) in the compiler instead of stdlib, which is why we have all those symbols in condsyms.nim used throughout stdlib.

Branching is common is user code too, eg in ~/.config/nim/config.nims when you want to use a flag defined in x.y.z for nim versions recent enough to have it:

# in ~/.config/nim/config.nims
import std/compilesettings
when nimVersionCT >= (1,3,7):
  switch("foo", "on")

nim_old --lib:lib main will use nimVersion = (1,3,9) but nimVersionCT = (1,3,7) if nim_old was compiled at (1,3,7) but --lib:lib points to a more recent stdlib eg (1,3,9)

#[
A bit hacky but allows using this before most of system.nim is defined
the only dependency is on NimMajor, NimMinor, NimPatch, `int`
]#
Copy link
Member

Choose a reason for hiding this comment

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

Seriously?

@Araq
Copy link
Member

Araq commented Jun 14, 2020

Sorry but rejected. Nobody else asks for these stunts all the time. Please ask yourself why that is.

@Araq Araq closed this Jun 14, 2020
@timotheecour
Copy link
Member Author

timotheecour commented Jun 14, 2020

@Araq well can you please re-open and I'll just remove everything except these 2 lines:

# compilesettings.nim
proc nimVersionCTImpl(): (major: int, minor: int, patch: int) {.compileTime.} = discard
const nimVersionCT* = nimVersionCTImpl()

plus the 2 lines in compiler/vmops.nim

which is the part I really need. Thanks

@Araq
Copy link
Member

Araq commented Jun 14, 2020

I'd rather keep the nimHasX way instead then. Otherwise the overlap with system.NimVersion is too dangerous, when we have two ways, it's easy to get it wrong. These things are subtle.

@timotheecour
Copy link
Member Author

overlap with system.NimVersion is too dangerous, when we have two ways, it's easy to get it wrong.

But it's right there in the name nimVersionCT, I don't see anything confusing about it and at some point users just have to read the docs; a client of compilesettings is likely to understand that kind of subtely, it's not like it's added to system.

I'd be happy to revisit this PR to strip out everything except the part needed to get the CT tuple.

@Araq
Copy link
Member

Araq commented Oct 27, 2020

It's not "confusing" but it is a choice and choices take up brain power.

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

Successfully merging this pull request may close these issues.

3 participants