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

Remove official ndebug variable support #563

Merged
merged 2 commits into from
Feb 18, 2015

Conversation

Aatch
Copy link
Contributor

@Aatch Aatch commented Jan 9, 2015

@huonw
Copy link
Member

huonw commented Jan 9, 2015

I'm in favour of movement along these lines; I find it weird to think that a --cfg flag will be used by the compiler for internal codegen choices, rather than just source-level stripping.

@SimonSapin
Copy link
Contributor

It can be useful to run tests with both debug_assert! and optimizations enabled (if running tests take a lot of time).

@huonw
Copy link
Member

huonw commented Jan 9, 2015

@SimonSapin I think the suggestion is to allow -C debug-assertions=true to override the opt-level-based default settings.

@SimonSapin
Copy link
Contributor

Ok, that sounds fine.

@Aatch Aatch mentioned this pull request Jan 9, 2015
@nikomatsakis
Copy link
Contributor

Yes, this makes sense. ndebug in my head has kind of been what you are actually proposing -- that is, a way to enable/disable expensive assertions.

@nikomatsakis
Copy link
Contributor

Thinking a bit more, I think it would be useful to have the ability to narrow down the list of modules for which debug-assertions are enabled, as well. I've wanted this in C++ from time to time (and achieved it, by playing with #undef NDEBUG and so on). I'd start by just having this controlled via the command-line, but eventually we may want in-code scoped annotations, much like what was proposed for integer overflow initially. My opinion on that, though, is that I'd prefer to take it slow and start simple; I feel we don't have enough experience to judge just how much control is really desired, and I want to avoid over-designing things. To that end, even this proposal of scoping by module is probably overboard: I think all I mean is that I'd like to leave room for specifying a list of modules or other information in the future (categories of debug-assertions and so on).

@brson
Copy link
Contributor

brson commented Jan 12, 2015

In favor of the general idea.

@brson
Copy link
Contributor

brson commented Jan 12, 2015

For context ndebug started out as an in-tree thing and sorta seeped into the outside world.

@sfackler
Copy link
Member

The debug! logging macro also disables itself if ndebug is set at the moment, but my restructuring of the log crate does away with that in favor of a more configurable system: https://github.com/sfackler/log-ng/blob/master/src/macros.rs


# Detailed design

The `debug_assertions` variable, the replacement for the `ndebug` variable, will be compiler
Copy link
Member

Choose a reason for hiding this comment

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

Just clarifying, but by variable do you mean cfg directive? For example, I would be able to do:

if cfg!(debug_assertions) { /* super-expensive code */ }

@brson
Copy link
Contributor

brson commented Feb 2, 2015

I have a minor concern that -C debug-assertions might not be the right place for this command line flag - it doesn't really affect code generation, at least in the current codebase (also --cfg debug_assertions has the same effect).

What other things might debug_assertions control? Does it affect integer overflow?

@huonw
Copy link
Member

huonw commented Feb 2, 2015

It seems like the flag could be more than just a boolean, but rather take a list of what to enable to allow fine-grained control, e.g. none, overflow-checks, debug_cfg,overflow-checks, all. (Where -C debug-assertions=debug_cfg acts like --cfg debug.)

@nikomatsakis
Copy link
Contributor

Just re-read the RFC again. I like it. I definitely like this part of it:

  • -O0 enables all optional debugging stuff
  • -O1 and higher disables all optional debugging stuff, but you can opt back into it

The only question then is how fine-grained to make the "opting back in". That is, do we want to distinguish debug-assertions from integer-overflow etc? I think we certainly want a simple way to say "put back in all the optional debugging stuff". Having more control also seems good but I'd be happy for that to come later too.

@huonw
Copy link
Member

huonw commented Feb 17, 2015

Sorry for bikeshedding... if we want this to apply to more than just debug_assert do we want to use a name other than -C debug-assertions?

@nikomatsakis
Copy link
Contributor

@huonw I don't have a strong opinion. I'm going to merge the RFC in any case, I think, and we can bikeshed independently and update the RFC/impl as we go. I think the important part is that we plan on integrating this more deeply in the compiler and that we plan on having the defaults be connected to the optimization level.

@nikomatsakis
Copy link
Contributor

The core team has decided to merge this RFC. There was no significant opposition, seems like a popular proposal all around. Some [minor] questions as to the precise command-line flags remain to be resolved [1, 2, 3], but the high-level idea of integrating ndebug more deeply into the compiler and tying it to the optimization flags is uncontroversial.

@nikomatsakis
Copy link
Contributor

Tracking issue: rust-lang/rust#22492

@nikomatsakis nikomatsakis merged commit 6ac0f88 into rust-lang:master Feb 18, 2015
nikomatsakis added a commit that referenced this pull request Feb 18, 2015
add in unresolved questions from the RFC discussion.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 4, 2015
This commit is an implementation of [RFC 563][rfc] which adds a new
`cfg(debug_assertions)` directive which is specially recognized and calculated
by the compiler. The flag is turned off at any optimization level greater than 1
and may also be explicitly controlled through the `-C debug-assertions`
flag.

[rfc]: rust-lang/rfcs#563

The `debug_assert!` and `debug_assert_eq!` macros now respect this instead of
the `ndebug` variable and `ndebug` no longer holds any meaning to the standard
library.

Code which was previously relying on `not(ndebug)` to gate expensive code should
be updated to rely on `debug_assertions` instead.

Closes rust-lang#22492
[breaking-change]
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 4, 2015
…kfelix

 This commit is an implementation of [RFC 563][rfc] which adds a new
`cfg(debug_assertions)` directive which is specially recognized and calculated
by the compiler. The flag is turned off at any optimization level greater than 1
and may also be explicitly controlled through the `-C debug-assertions`
flag.

[rfc]: rust-lang/rfcs#563

The `debug_assert!` and `debug_assert_eq!` macros now respect this instead of
the `ndebug` variable and `ndebug` no longer holds any meaning to the standard
library.

Code which was previously relying on `not(ndebug)` to gate expensive code should
be updated to rely on `debug_assertions` instead.

Closes rust-lang#22492
[breaking-change]
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 5, 2015
This commit is an implementation of [RFC 563][rfc] which adds a new
`cfg(debug_assertions)` directive which is specially recognized and calculated
by the compiler. The flag is turned off at any optimization level greater than 1
and may also be explicitly controlled through the `-C debug-assertions`
flag.

[rfc]: rust-lang/rfcs#563

The `debug_assert!` and `debug_assert_eq!` macros now respect this instead of
the `ndebug` variable and `ndebug` no longer holds any meaning to the standard
library.

Code which was previously relying on `not(ndebug)` to gate expensive code should
be updated to rely on `debug_assertions` instead.

Closes rust-lang#22492
[breaking-change]
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 6, 2015
…kfelix

 This commit is an implementation of [RFC 563][rfc] which adds a new
`cfg(debug_assertions)` directive which is specially recognized and calculated
by the compiler. The flag is turned off at any optimization level greater than 1
and may also be explicitly controlled through the `-C debug-assertions`
flag.

[rfc]: rust-lang/rfcs#563

The `debug_assert!` and `debug_assert_eq!` macros now respect this instead of
the `ndebug` variable and `ndebug` no longer holds any meaning to the standard
library.

Code which was previously relying on `not(ndebug)` to gate expensive code should
be updated to rely on `debug_assertions` instead.

Closes rust-lang#22492
[breaking-change]
@Centril Centril added the A-cfg Conditional compilation related proposals & ideas label Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cfg Conditional compilation related proposals & ideas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants