This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Resolve compiler warnings in LCM/GCD tests
ClosedPublic

Authored by BillyONeal on Apr 20 2017, 1:56 PM.

Details

Summary

Resolve integer overflow warnings in GCD and LCM tests.

lcm.pass.cpp:
19: Update headers to that actually used in the test.
41: test0 was triggering narrowing warnings for all callers, because the
inputs were always ints, but some of the explicit template arguments were
smaller than that. Instead, have this function accept ints and static_cast
explicitly to the types we want before calling std::lcm.
47: Replace unnecessary ternary.
55: Use foo_t instead of typename foo<>::type
111/116: intX_t were not std::qualified but only <cfoo> headers were included.
141: C1XX has a bug where it interprets 2147483648 as unsigned int. Then the
negation trips "negation of unsigned value, result still unsigned" warnings.
Perma-workaround this issue by saying INT_MIN, which better documents the
intended behavior and avoids triggering warnings on C1XX.

gcd.pass.cpp:
Same changes as lcm.pass.cpp but for GCD.

Diff Detail

Event Timeline

BillyONeal created this revision.Apr 20 2017, 1:56 PM
BillyONeal retitled this revision from Resolve compiler warnings in LCM/GCD tests to [libcxx] [test] Resolve compiler warnings in LCM/GCD tests.Apr 20 2017, 2:04 PM
BillyONeal added a subscriber: cfe-commits.
BillyONeal updated this revision to Diff 96231.Apr 21 2017, 2:12 PM

Stephan had some code review comments :)

rsmith added a subscriber: rsmith.Apr 21 2017, 2:35 PM
rsmith added inline comments.
test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
47

Is there a reason to recompute the type here rather than using Output?

BillyONeal added inline comments.Apr 22 2017, 12:15 PM
test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
47

Hmm... I think I was concerned about it looking like the cast o the output type was an assertion that the return type was in fact of that type (which isn't true). But that should be taken care of by the previous static_asserts.

Hi folks, any update on this or is just fixing @rsmith's comment OK?

Thanks!

CaseyCarter added inline comments.
test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
139

INT_MIN is in <climits> .

EricWF added inline comments.May 4 2017, 1:19 AM
test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
141

std::common_type here please. This test runs in C++03 where we don't have std::foo_t.

BillyONeal added inline comments.May 4 2017, 2:01 AM
test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
141

C++17 library features in C++03?!? :(

OK, will fix.

BillyONeal updated this revision to Diff 98205.May 8 2017, 2:02 PM

Resolved CR comments

BillyONeal marked 5 inline comments as done.May 8 2017, 2:02 PM
EricWF accepted this revision.May 8 2017, 2:28 PM

LGTM, preferably with the suggested cleanups.

test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
42–43

Nit but this seems much cleaner and more readable without the casts.

auto value1 = static_cast<Input1>(in1);
auto value2 = static_cast<Input2>(in2);
static_assert(std::is_same_v<Output, decltype(std::gcd(value1, value2))>, "");
static_assert(std::is_same_v<Output, decltype(std::gcd(value2, value1))>, "");
assert(static_cast<Output>(out) == std::gcd(value1, value2));
return true;

Also just use assert instead of std::abort()

141

Sorry my bad. This test is truely C++17 only. I missed the // UNSUPPORTED line originally. Feel free to run wild with C++17 features.

test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
41–46

Same suggestion as on the above test.

This revision is now accepted and ready to land.May 8 2017, 2:28 PM
BillyONeal added inline comments.May 8 2017, 2:47 PM
test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
42–43

I was under the impression the abort dance was because assert isn't constexpr.

EricWF added inline comments.May 8 2017, 2:48 PM
test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
42–43

assert is constexpr for the exact same reason the abort dance was constexpr.

And I think somebody else was under the same misapprehension when they wrote this test.

BillyONeal updated this revision to Diff 98219.May 8 2017, 3:02 PM
BillyONeal marked 2 inline comments as done.
BillyONeal edited the summary of this revision. (Show Details)

Do Eric's suggestions.

BillyONeal closed this revision.May 8 2017, 3:08 PM