Add a flag to build profiles to enable overflow checking #2262

Closed
larsbergstrom opened this Issue Jan 6, 2016 · 4 comments

Comments

Projects
None yet
4 participants
@larsbergstrom

Would it be possible to add a flag to the build profiles:
http://doc.crates.io/manifest.html#the-profile-sections

That allows us to turn overflow checking on? (that is, enable -Z force-overflow-checks=on)

While we probably wouldn't ship Servo this way, being able to turn it on in our CI systems would catch a bunch of bugs without requiring us to spin up a parallel set of debug-only builders.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 11, 2016

Member

There's definitely an interesting question of this I think of where the configuration should go. For something like this it may make sense actually in a few locations. Profiles aren't always the best place because there's only one global compilation profile (chosen by the root crate), which overrides local configuration. So for a use case of "I want overflow checks always enabled in this crate" it's not quite appropriate, but for a CI system it may be appropriate.

That being said Cargo profiles aren't great for CI systems right now as it involves editing Cargo.toml where it's easier to just pass a CLI flag, so there's also that to consider.

Overall, though, I'm very wary about stabilizing any usage of -Z options in Cargo as they should likely become full-fledged -C options first.

Member

alexcrichton commented Jan 11, 2016

There's definitely an interesting question of this I think of where the configuration should go. For something like this it may make sense actually in a few locations. Profiles aren't always the best place because there's only one global compilation profile (chosen by the root crate), which overrides local configuration. So for a use case of "I want overflow checks always enabled in this crate" it's not quite appropriate, but for a CI system it may be appropriate.

That being said Cargo profiles aren't great for CI systems right now as it involves editing Cargo.toml where it's easier to just pass a CLI flag, so there's also that to consider.

Overall, though, I'm very wary about stabilizing any usage of -Z options in Cargo as they should likely become full-fledged -C options first.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Jul 28, 2016

Contributor

Perhaps something like overflower could help?

Contributor

llogiq commented Jul 28, 2016

Perhaps something like overflower could help?

@froydnj

This comment has been minimized.

Show comment
Hide comment
@froydnj

froydnj Feb 22, 2017

Contributor

Now that rust-lang/rust#33134 is stabilizing -C overflow-checks, we can add this, correct? I know in rust-lang/rfcs#1535, @nrc expressed a preference that forthcoming Cargo support for arbitrary flags would be used instead of adding another boolean to profiles...how close is that arbitrary flags support @alexcrichton ?

Contributor

froydnj commented Feb 22, 2017

Now that rust-lang/rust#33134 is stabilizing -C overflow-checks, we can add this, correct? I know in rust-lang/rfcs#1535, @nrc expressed a preference that forthcoming Cargo support for arbitrary flags would be used instead of adding another boolean to profiles...how close is that arbitrary flags support @alexcrichton ?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 22, 2017

Member

@froydnj let's stick to booleans for now, unforunately arbitrary flags may take awhile :(

Member

alexcrichton commented Feb 22, 2017

@froydnj let's stick to booleans for now, unforunately arbitrary flags may take awhile :(

froydnj added a commit to froydnj/cargo that referenced this issue Apr 7, 2017

add `overflow-checks` field to profiles
...and pass -C overflow-checks to the compiler when necessary.

Fixes #2262.

froydnj added a commit to froydnj/cargo that referenced this issue Apr 7, 2017

add `overflow-checks` field to profiles
...and pass `-C overflow-checks` to the compiler when necessary.

Fixes #2262.

froydnj added a commit to froydnj/cargo that referenced this issue Apr 11, 2017

add `overflow-checks` field to profiles
...and pass `-C overflow-checks` to the compiler when necessary.

Fixes #2262.

froydnj added a commit to froydnj/cargo that referenced this issue Apr 14, 2017

add `overflow-checks` field to profiles
...and pass `-C overflow-checks` to the compiler when necessary.

Fixes #2262.

bors added a commit that referenced this issue Apr 18, 2017

Auto merge of #3908 - froydnj:overflow-checks, r=alexcrichton
add `overflow-checks` field to profiles

...and pass `-C overflow-checks` to the compiler when necessary.

Fixes #2262.

bors added a commit that referenced this issue Apr 18, 2017

Auto merge of #3908 - froydnj:overflow-checks, r=alexcrichton
add `overflow-checks` field to profiles

...and pass `-C overflow-checks` to the compiler when necessary.

Fixes #2262.

@bors bors closed this in #3908 Apr 18, 2017

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