Page MenuHomePhabricator

[clang-tidy] Improving narrowing conversions
ClosedPublic

Authored by gchatelet on Oct 22 2018, 2:20 AM.

Details

Summary

Newly flagged narrowing conversions:

  • integer to narrower signed integer (this is compiler implementation defined),
  • integer - floating point narrowing conversions,
  • floating point - integer narrowing conversions,
  • constants with narrowing conversions (even in ternary operator).

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
JonasToth added inline comments.Nov 12 2018, 8:19 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
30 ↗(On Diff #173633)

I would make this on by default. It is a valid diagnostic and people using this check might be surprised they have to activate it.

169 ↗(On Diff #173633)

s/clang tidy already/clang-tidy check already/ (not sure if clang-tidy-check is correcter from english point of view.)

206 ↗(On Diff #173633)

i think llvm_unreachable would fit better.

299 ↗(On Diff #173633)

Why is the return here true even without diagnostic?

344 ↗(On Diff #173633)

llvm_unreachable

gchatelet updated this revision to Diff 173836.Nov 13 2018, 5:39 AM
gchatelet marked 6 inline comments as done.
  • Address comments
gchatelet added inline comments.Nov 13 2018, 5:40 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
299 ↗(On Diff #173633)

Good catch

gchatelet updated this revision to Diff 173844.Nov 13 2018, 6:57 AM
  • Remove debugging leftover
JonasToth accepted this revision.Nov 13 2018, 7:14 AM

So, finally LGTM :)
Please give @aaron.ballman a chance to comment, as he reviewed too.

Thanks for your patch!

This revision is now accepted and ready to land.Nov 13 2018, 7:14 AM

So, finally LGTM :)
Please give @aaron.ballman a chance to comment, as he reviewed too.

Thanks for your patch!

Thank you for bearing with me.
Waiting for input from @aaron.ballman

aaron.ballman added inline comments.Nov 14 2018, 5:31 AM
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
79–81 ↗(On Diff #173844)

This is not a narrowing conversion -- there should be no diagnostic here. See [dcl.init.list]p7.

99 ↗(On Diff #173844)

Missing tests involving literals, which are not narrowing conversions. e.g., float f = 1ULL; should not diagnose.

174 ↗(On Diff #173844)

This is only true on architectures where char is defined to be signed char rather than unsigned char.

188 ↗(On Diff #173844)

This is only narrowing if sizeof(int) < sizeof(long) which is not the case on all architectures.

211 ↗(On Diff #173844)

Whatttt? How does one narrow from an unsigned 32-bit number to a signed 64-bit number?

228 ↗(On Diff #173844)

This may be narrowing on an implementation that defines char to be unsigned.

232 ↗(On Diff #173844)

This may be fine on implementations that define char to be unsigned.

gchatelet updated this revision to Diff 174044.Nov 14 2018, 8:06 AM
gchatelet marked 7 inline comments as done.
  • State char signess clearly

Thx for the review. I have two suggestions in the comments let me know what you think.

test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
79–81 ↗(On Diff #173844)

Thx for pointing this out.
I would like to keep the diagnostic (without mentioning the narrowing) because there is no reason to write 2.0 instead of 2.0f. WDYT?

99 ↗(On Diff #173844)

This is handled on l.262

174 ↗(On Diff #173844)

This is taken care of. The test fails when adding -funsigned-char.
The current test is executed with -target x86_64-unknown-linux which should imply -fsigned-char. Anyways, I'll add -fsigned-char to make sure this is obvious.

188 ↗(On Diff #173844)

Yes this is taken care of in the code, the size of the type must be smaller.
It works here because the test runs with -target x86_64-unknown-linux

211 ↗(On Diff #173844)

unsigned long is unsigned 64-bit so it does not fit in signed 64-bit.
https://godbolt.org/z/VnmG0s

228 ↗(On Diff #173844)

Same as previous comments. Would it make sense to create a different test with -funsigned-char to check these behaviors?

232 ↗(On Diff #173844)

ditto

aaron.ballman added inline comments.Nov 14 2018, 8:32 AM
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
79–81 ↗(On Diff #173844)

I see no reason to warn on a literal value that isn't changed as a result of the notional narrowing. The standard clearly defines the behavior, so I think that would be needlessly chatty.

99 ↗(On Diff #173844)

Ah, I missed that, thank you!

174 ↗(On Diff #173844)

Ah, thank you! Can you also pass -funsigned-char to test the behavior is as expected?

188 ↗(On Diff #173844)

Please add a test for another target where this is not the case.

211 ↗(On Diff #173844)

unsigned long is not 64-bit on all targets, and that should be tested. https://godbolt.org/z/VQx-Yy

gchatelet updated this revision to Diff 174374.Nov 16 2018, 8:08 AM
gchatelet marked 13 inline comments as done.
  • Addressed comments
gchatelet added inline comments.Nov 16 2018, 8:08 AM
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
79–81 ↗(On Diff #173844)

Done. Plus added som tests.

aaron.ballman added inline comments.Nov 16 2018, 10:14 AM
test/clang-tidy/cppcoreguidelines-narrowing-conversions-long-is-32bits.cpp
4–10 ↗(On Diff #174374)

Add some static_asserts for these bit sizes?

test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
42–44 ↗(On Diff #174374)

I don't think these should diagnose. They're both harmless as the literal values are exactly representable in the destination type.

gchatelet updated this revision to Diff 175030.Nov 22 2018, 2:47 AM
gchatelet marked an inline comment as done.
  • Refactored the code to make it simpler, added more tests
gchatelet marked 2 inline comments as done.Nov 22 2018, 2:58 AM
gchatelet added inline comments.
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
42–44 ↗(On Diff #174374)

Yes indeed they are harmless but why not write the correct literal in the first place?
I'm keeping these warnings for now. Let me know if you feel strongly about it.

gchatelet updated this revision to Diff 175033.Nov 22 2018, 3:33 AM
gchatelet marked an inline comment as done.
  • Reverting the WarnOnFloatingPointNarrowingConversion option to be on by default
aaron.ballman added inline comments.Nov 23 2018, 7:20 AM
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
42–44 ↗(On Diff #174374)

Yes indeed they are harmless but why not write the correct literal in the first place?

Why force the user to modify their code when the behavior will be exactly the same as before?

I'm keeping these warnings for now. Let me know if you feel strongly about it.

I don't feel very strongly on this case, but it seems needlessly chatty to me to diagnose code that's not incorrect. For instance, this code could exist in a (system) header file that the user doesn't have the ability to correct.

gchatelet updated this revision to Diff 175139.Nov 23 2018, 1:10 PM
  • Added a new option warn about wrong type literals
gchatelet marked an inline comment as done.Nov 23 2018, 1:15 PM
gchatelet added inline comments.
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
42–44 ↗(On Diff #174374)

I've added a WarnOnCastingLiterals option to display them.
It's on by default.
Let me know if you have a better name, I'm not super happy with it.
Let me know if you think it should be off by default.

aaron.ballman added inline comments.Nov 24 2018, 5:48 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
31 ↗(On Diff #175139)

I think the concept here is more broad than the option name suggests -- this is really a "pedantic mode" for this feature, because there is a notional narrowing but no semantic difference. I would name the option PedanticMode and have it off-by-default, personally.

127 ↗(On Diff #175139)

integer -> integers

242–244 ↗(On Diff #175139)

You should cite where this quotation comes from (e.g., [foo.bar]p2)

259–263 ↗(On Diff #175139)

Why is this function needed at all?

311–315 ↗(On Diff #175139)

Why is this function needed at all?

325 ↗(On Diff #175139)

[dcl.init.list]p7.2 instead of a hyperlink.

372 ↗(On Diff #175139)

What variables are created here?

373 ↗(On Diff #175139)

short ciruit -> short-circuit

docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
59 ↗(On Diff #175139)

drop the "so"

61 ↗(On Diff #175139)

do not -> does not
as such -> and so

62 ↗(On Diff #175139)

what is the semantic -> what the semantics are

gchatelet updated this revision to Diff 175203.Nov 26 2018, 1:05 AM
gchatelet marked 16 inline comments as done.
  • Addressing comments

Thank you for bearing with me @aaron.ballman.

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
259–263 ↗(On Diff #175139)

The two empty functions are placeholders so the cases handled by ImplicitCast and BinaryOp are the same.
I'll add some documentation.

311–315 ↗(On Diff #175139)

Ditto.

372 ↗(On Diff #175139)

Leftover. I rewrote the comment.

gchatelet updated this revision to Diff 175204.Nov 26 2018, 1:21 AM
  • Fixing documentation.

I think we're almost there -- I had a few outstanding questions about the config options in the tests and making sure we cover all the cases.

test/clang-tidy/cppcoreguidelines-narrowing-conversions-castingliterals-option.cpp
3 ↗(On Diff #175204)

No need for this setting?

test/clang-tidy/cppcoreguidelines-narrowing-conversions-long-is-32bits.cpp
2–5 ↗(On Diff #175204)

No need for any of these settings?

test/clang-tidy/cppcoreguidelines-narrowing-conversions-narrowingfloatingpoint-option.cpp
2–5 ↗(On Diff #175204)

You can drop these lines, I believe, because they match the default values.

test/clang-tidy/cppcoreguidelines-narrowing-conversions-unsigned-char.cpp
2–5 ↗(On Diff #175204)

No need for these settings?

gchatelet updated this revision to Diff 175231.Nov 26 2018, 5:20 AM
gchatelet marked 4 inline comments as done.
  • Removed redundant options in regression tests
This revision was automatically updated to reflect the committed changes.