add overflow-checks field to profiles

[froydnj]

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

Fixes #2262.

[rust-highfive]

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

[froydnj]

I am not super-happy about the logic in cargo_rustc/mod.rs; that seems like the sort of thing will have to be duplicated by anybody who wants to consume Cargo's output. I understand that they already have to do that with debug-assertions, but adding several extra cases might be taking it too far. I'm not really sure what a good alternative would be.

[alexcrichton]

Thanks!

The logic here was mostly just to make -v pretty, I think we can always resort to just unconditionally passing these flags perhaps.

Could you also add a test to ensure this is plumbed through and enabled?

[froydnj]

I think we can always resort to just unconditionally passing these flags perhaps.

I assume this means using the same logic that debug-assertions already uses.

Could you also add a test to ensure this is plumbed through and enabled?

@alexcrichton Do you mean something different than the tests in tests/build.rs? Or do you mean testing a Cargo.toml with explicit declarations of `overflow-checks?

[alexcrichton]

Oh not precisely, we try to make the command line "pretty" by not passing -Cdebug-assertions=on when compiling in debug mode. That is, Cargo assumes that -C opt-level=0 is the default (doesn't need to be passed) and implies that debug assertions on. Cargo then treats opt-level 0 and debug assertions on as "pass no flags to rustc". That makes the logic here a little odd (with all the various implications), so I'm fine either way (whichever you prefer)

Yeah let's add a test to something like tests/build.rs or tests/test.rs (or anywhere really). Mostly just build a program in release mode with an overflow and make sure it panics.

[froydnj]

OK, updated patch pushed with a release-mode, overflow-checks test. I left the logic that adds -C overflow-checks as-is, because a tidy -v command line is better than a cluttered one.

[alexcrichton]

Looks great to me! I think though the test needs to be gated on nightly only?

[froydnj]

Looks great to me! I think though the test needs to be gated on nightly only?

Ah, good point! How does that work when -C overflow-checks makes it into stable, though...it won't get tested there, then?

[alexcrichton]

@froydnj oh we just delete the "if not nightly skip test" checks at that point (eventually ...)

[froydnj]

oh we just delete the "if not nightly skip test" checks at that point (eventually ...)

Hey, OK!

[alexcrichton]

@bors: r+

[bors]

📌 Commit 803d748 has been approved by alexcrichton

[bors]

⌛ Testing commit 803d748 with merge b94fbda...

[bors]

💔 Test failed - status-travis

[alexcrichton]

That seems... odd!

Let's see if it's spurious...

@bors: retry

[bors]

⌛ Testing commit 803d748 with merge 434e7a8...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 434e7a8 to master...