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

Document defines #124

Closed
iffy opened this issue Jan 29, 2019 · 8 comments
Closed

Document defines #124

iffy opened this issue Jan 29, 2019 · 8 comments
Labels

Comments

@iffy
Copy link

iffy commented Jan 29, 2019

Document defines

Abstract

I propose that a mechanism be created to uniquely document defines (the flags passed to the compiler with -d).

  1. Uniquely: The compiler fails if documentation has been provided more than once for a define flag
  2. Document: Add some method for extracting the documentation of all available defines. (e.g. in nim doc maybe?)

I don't know the best syntax for this, but something like:

{.docdefine: ["logGC", "Enable verbose GC logging"] .}

when defined(logGC):
    ...

Motivation

  1. It's too easy to add defines that are undiscoverable by other users.
  2. It's too easy to accidentally use a define that already has another use (that you're not aware of)
  3. It's easy for the documentation about define flags to become out of date. Documenting near the defines use might make it easier to keep documentation up to date.
  4. Users of libraries/stdlib benefit by being able to enumerate possible define flags
  5. Authors of libraries/stdlib benefit by being able to document possible define flags without having to add extra documentation in distant files or docstrings.
  6. Authors also benefit by not being able to accidentally using an already-defined define
  7. Authors also benefit by more easily being able to reuse existing definitions (e.g. should I use "mac", "macOS" or "macOSX"?)

Downsides

  • It might be nice to reuse a flag and add documentation to it for your particular use case.
  • It might make the compiler slower

Examples

# mylib.nim
{.docdefine: ["logGC", "Enable graphing calculator logging"] .}
{.docdefine: ["mylib_verboseLogging", "Enable verbose output"] .}

when defined(logGC):
    echo "Logging graphing calculator"

when defined(mylib_verboseLogging):
    echo "VERBOSE output"

A potential documentation tool

$ nim-show-defines mylib.nim
logGC                   Enable verbose GC logging   [DUPLICATE]
mylib_verboseLogging    Enable verbose output

Documenting all defines imported by mylib.nim:

$ nim-show-defines --all mylib.nim
logGC                   Enable verbose GC logging   [DUPLICATE]
mylib_verboseLogging    Enable verbose output
release                 Compile in release mode
useGCAssert             Enable GC assertions
useSysAssert            Enable System assertions
logGC                   Enable verbose GC logging   [DUPLICATE]
...

Trying to compile fails because logGC is already used for something:

$ nim c mylib.nim
[ERROR] conflicting logGC definitions in:
mylib.nim:2: {.docdefine: ["logGC", "Enable graphing calculator logging"] .}
../../lib/system/gc.nim:22: {.docdefine: ["logGC", "Enable GC logging"] .}

Backward incompatibility

My track record for detecting backward-incompatibility isn't very good, but since the definitions aren't currently being used, backward compatibility will only be introduced as each individual flag is documented.

@iffy
Copy link
Author

iffy commented Jan 29, 2019

cc/ @timotheecour

@timotheecour
Copy link
Member

timotheecour commented Jan 29, 2019

I completely agree with the general idea; I have some comments on specific aspects.

Note: it's more like a declaration rather than a documentation (documentation is a side effect), just like in:
var a: A ## some great variable where the doc is a side effect but a is declared (before being used later on)

It might make the compiler slower

not in any measureable way, no

It might be nice to reuse a flag and add documentation to it for your particular use case.

that would only be the case if it's used in package A and package B and A and B don't coexist in the same application; as soon as they are, you have a potential bug; so that's not a valid argument against your proposal.

A potential documentation tool

we don't want yet another binary for many reasons (costly to build, has to stay in sync with compiler etc); instead this is a more scalable approach: #123 nim c --dump:output.json main.nim (I temporarily closed it while I'm fleshing out details); you can use it flexibly eg:

nim c --compileOnly --dump:output.json main.nim and it does the exact same work as the compiler does, by definition; note that your nim-show-defines would have to also do the exact same work as the compiler in order to be correct, eg:

when someComplexCalculation() > 10:
  {.docdefine:["foo1", "bar1"].}
when defined(osx):
  {.docdefine:["foo2", "bar2"].}
  • This also should be accessible in code (as opposed to via a cmd line tool)

Note: we already have nim dump but that's different; that's to enumerate the defined that are passed to compiler (either implicitly via defineSymbol, or through cmd line, or through config files):

nim dump . | jq .defined_symbols
[
  "nimHasWarningUseBase",
  "nimSymKind",
  "nimHasWarningProveIndex",
  "nimHasimplicitDeref",

note: instead of [DUPLICATE] as in your example, you'll get a regular compiler error if there's a duplicate docdefine

note

  • your proposal should also cover --define:SYMBOL:VAL syntax (you only covered --define:SYMBOL)

issue warning for defined(foo) without docdefine

  • I'd go a step further than your proposal:
    if foo.nim contains defined(foo) without a corresponding docdefine in scope, issue a warning

Note: this warning is for code using defined(foo) without having a corresponding docdefine; it's not for a user passing -d:foo on cmd line without a corresponding docdefine; that's a different story

migration path

# inside std/bar.nim

# before
when defined(foobar): baz # compiler issues warning: `foobar` not declared in scope

# after
import std/defines # puts `foobar` in scope; a file containing a bunch of `docdefine`; maybe some are under system.nim
from std/defines import foobar # also works
when defined(foobar): baz

@GULPF
Copy link
Member

GULPF commented Jan 29, 2019

I have an alternative proposal that I think is significantly easier:

Currently {.strdefine.} and {.intdefine.} can be used for attaching a value to a compile time value. If we introduce a {.booldefine.} we could let --define:foo be a shorthand for --define:foo=true. After that, only two changes are needed to ensure safety:

  • Issue a compiler warning when {.xdefine.} is used multiple times with the same identifier name.
  • Issue a compiler warning for defined(X) if X isn't a const with a {.xdefine.} pragma

Note that the issue of attaching documentation would now just be a matter of adding a normal doc comment to the symbol with a {.xdefine.} pragma.

The original proposal has the issue that it's not clear if logGC is a proper symbol or not. The syntax from std/defines import foobar would indicate that it's a proper symbol, but that opens up all kinds of questions (can the symbol be shadowed? what type does the symbol have? etc).

@Araq
Copy link
Member

Araq commented Jan 29, 2019

Please note that defines are documented here:

https://nim-lang.org/docs/nimc.html#additional-compilation-switches

There are lots of others, but they almost all use the nim... prefix which is reserved and shouldn't be used by Nimble packages.

@timotheecour
Copy link
Member

timotheecour commented Jan 30, 2019

@GULPF I really like your proposal;

{.xdefine.}

I'm assuming u mean this as a place holder fo {bool,int,str}define, right?

they almost all use the nim... prefix which is reserved and shouldn't be used by Nimble packages

  • there are a lot of define flags not mentioned in that table that in addition don't start with nim eg (defined(js), defined(cpp), defined(clang) etc
    Furthermore, it's not just for Nim repo, that feature is useful for nimble packages that want to use defined(foo) and not clash with each other

so... let's just go with @GULPF 's proposal then?

@GULPF
Copy link
Member

GULPF commented Jan 30, 2019

I'm assuming u mean this as a place holder fo {bool,int,str}define, right?

Yes

@github-actions
Copy link

github-actions bot commented Jun 3, 2023

This RFC is stale because it has been open for 1095 days with no activity. Contribute a fix or comment on the issue, or it will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 3, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 11, 2023
@metagn
Copy link
Contributor

metagn commented Jun 11, 2023

Again fwiw we can use strdefine/booldefine constants etc and document them now

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

No branches or pull requests

5 participants