This is an archive of the discontinued LLVM Phabricator instance.

[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

Did you run this code over a real-world code-base and did you find new stuff and/or false positives or the like?

Yes I did run it over our code base. I didn't find false positive but 98% of the warnings are from large generated lookup table initializers, e.g. const static float kTable[] = {0.0, 2.0, ... };
Since every number in the list triggers the warning, it accounts for most of them.

I scrutinized a few hundreds of the rest: none were actual bugs (although it's hard to tell sometimes), most are legit like float value = 0.0; but I also found some oddities from generated headers.

To me the warnings are useful and if it were my code I'd be willing to fix them. That said, I'd totally understand that many people would find them useless or annoying.
What do you think? Shall we still commit this as is?

Did you run this code over a real-world code-base and did you find new stuff and/or false positives or the like?

Yes I did run it over our code base. I didn't find false positive but 98% of the warnings are from large generated lookup table initializers, e.g. const static float kTable[] = {0.0, 2.0, ... };
Since every number in the list triggers the warning, it accounts for most of them.

I scrutinized a few hundreds of the rest: none were actual bugs (although it's hard to tell sometimes), most are legit like float value = 0.0; but I also found some oddities from generated headers.

To me the warnings are useful and if it were my code I'd be willing to fix them. That said, I'd totally understand that many people would find them useless or annoying.
What do you think? Shall we still commit this as is?

It would be nice to know how many new findings does this patch introduce (number of findings before the patch vs after). If it is not too much, it is fine the commit as it is.
I'd suggest to run the check on llvm code repository (using clang-tidy/tool/run-clang-tidy.py, and only enable cppcoreguidelines-narrowing-conversions).

Did you run this code over a real-world code-base and did you find new stuff and/or false positives or the like?

Yes I did run it over our code base. I didn't find false positive but 98% of the warnings are from large generated lookup table initializers, e.g. const static float kTable[] = {0.0, 2.0, ... };
Since every number in the list triggers the warning, it accounts for most of them.

I scrutinized a few hundreds of the rest: none were actual bugs (although it's hard to tell sometimes), most are legit like float value = 0.0; but I also found some oddities from generated headers.

To me the warnings are useful and if it were my code I'd be willing to fix them. That said, I'd totally understand that many people would find them useless or annoying.
What do you think? Shall we still commit this as is?

It would be nice to know how many new findings does this patch introduce (number of findings before the patch vs after). If it is not too much, it is fine the commit as it is.
I'd suggest to run the check on llvm code repository (using clang-tidy/tool/run-clang-tidy.py, and only enable cppcoreguidelines-narrowing-conversions).

I'll get back with some numbers.

In the meantime I found this one which is interesting
https://github.com/intel/ipmctl/blob/master/src/os/efi_shim/lnx_efi_api.c#L45
spec.tv_nsec (which is signed long) is converted to double and back to int64. There surely can be some loss in the process since double can only express 2^52 integers (2^52ns is about 52 days)

That one looks interesting :)

Am 25.10.18 um 17:13 schrieb Guillaume Chatelet via Phabricator:

gchatelet added a comment.

In https://reviews.llvm.org/D53488#1275786, @hokein wrote:

In https://reviews.llvm.org/D53488#1275750, @gchatelet wrote:

In https://reviews.llvm.org/D53488#1274205, @JonasToth wrote:

Did you run this code over a real-world code-base and did you find new stuff and/or false positives or the like?

Yes I did run it over our code base. I didn't find false positive but 98% of the warnings are from large generated lookup table initializers, e.g. const static float kTable[] = {0.0, 2.0, ... };
Since every number in the list triggers the warning, it accounts for most of them.

I scrutinized a few hundreds of the rest: none were actual bugs (although it's hard to tell sometimes), most are legit like float value = 0.0; but I also found some oddities https://github.com/ARM-software/astc-encoder/blob/master/Source/vectypes.h#L13999 from generated headers.

To me the warnings are useful and if it were my code I'd be willing to fix them. That said, I'd totally understand that many people would find them useless or annoying.
What do you think? Shall we still commit this as is?

It would be nice to know how many new findings does this patch introduce (number of findings before the patch vs after). If it is not too much, it is fine the commit as it is.
I'd suggest to run the check on llvm code repository (using clang-tidy/tool/run-clang-tidy.py, and only enable cppcoreguidelines-narrowing-conversions).

I'll get back with some numbers.

In the meantime I found this one which is interesting
https://github.com/intel/ipmctl/blob/master/src/os/efi_shim/lnx_efi_api.c#L45
spec.tv_nsec (which is signed long) is converted to double and back to int64. There surely can be some loss in the process since double can only express 2^52 integers (2^52ns is about 52 days)

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D53488

gchatelet updated this revision to Diff 171692.Oct 30 2018, 7:23 AM
  • Add more checks, disable bool implicit casts warning, enable ternary operator warnings.

So I've updated the code to add more narrowing conversions.
It now tests constant expressions against receiving type, do not warn about implicit bool casting, handles ternary operator with constant expressions.

I ran it on our code base: results look legit.
I'll ping back here with numbers for llvm once I figured out how to build the database the tool is complaining about :)

gchatelet added a comment.EditedOct 30 2018, 8:06 AM

So I ran llvm/tools/clang/tools/extra/clang-tidy/tool/run-clang-tidy.py
The version with all checks produces 65664 unique findings.
The version restricted to cppcoreguidelines-narrowing-conversions produces 4323 unique findings. That's about +7% of findings.
I can post some random ones if you're interested.

JonasToth requested changes to this revision.Oct 30 2018, 8:44 AM

because this now diverged quite a bit i will revert the lgtm for now and take a longer look at the new patch.
The numbers you reported sound good.

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
32 ↗(On Diff #171692)

This matcher seems no to be used, did I overlook sth?

docs/ReleaseNotes.rst
77 ↗(On Diff #171692)

that seems to be unrelated

This revision now requires changes to proceed.Oct 30 2018, 8:44 AM
JonasToth added inline comments.Oct 30 2018, 8:45 AM
docs/ReleaseNotes.rst
77 ↗(On Diff #171692)

sry for noise, this was part of the history diffs!

Szelethus added inline comments.
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
14 ↗(On Diff #171692)

Is APInt used anywhere?

gchatelet updated this revision to Diff 171717.Oct 30 2018, 9:15 AM
gchatelet marked 4 inline comments as done.
  • Remove leftover

Here are 15 random ones from llvm synced at 8f9fb8bab2e9b5b27fe40d700d2abe967b99fbb5.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3802:31: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
tools/dsymutil/MachOUtils.cpp:330:10: warning: narrowing conversion from 'unsigned long' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/Target/X86/X86ISelDAGToDAG.cpp:1237:14: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
tools/llvm-objdump/MachODump.cpp:7998:12: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/CodeGen/MachinePipeliner.cpp:3268:11: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/CodeGen/MIRParser/MIRParser.cpp:823:41: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/Analysis/MemoryBuiltins.cpp:610:59: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/Analysis/ValueTracking.cpp:2291:21: warning: narrowing conversion from 'unsigned long' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/Target/ARM/ARMLoadStoreOptimizer.cpp:1470:43: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2300:14: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/Target/X86/X86TargetTransformInfo.cpp:2590:27: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/Target/ARM/ARMFrameLowering.cpp:1770:27: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/Target/ARM/ARMConstantIslandPass.cpp:514:22: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/Transforms/Vectorize/LoopVectorize.cpp:5500:13: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1199:54: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions]
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
14 ↗(On Diff #171692)

Good catch thx.

32 ↗(On Diff #171692)

It's a leftover indeed thx for noticing.

JonasToth added inline comments.Oct 30 2018, 9:15 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
25 ↗(On Diff #171692)

I think this comment is now outdated.

42 ↗(On Diff #171692)

Here and in the matcher below:

isInTemplateInstantiation is a wide range, if you match only on isTypeDependent() it would remove the false negatives from templates but still catch clear cases that are within a template function.

With the current implementation non-type-templates would be ignored as well.
IMHO that could be done in a follow-up to keep the scope on one thing.

66 ↗(On Diff #171692)

Please enclose that struct with an anonymous namespace

85 ↗(On Diff #171692)

please remove that const as its uncommon to qualify values in LLVM code.

92 ↗(On Diff #171692)

Maybe repleace One with 1.0? It sounds slightly weird with the following Ones

118 ↗(On Diff #171692)

what about value dependentness? That is resolveable, but i believe ignored here

125 ↗(On Diff #171692)

narrowing to bool is not nice either. I would prefer diagnosing these too.

132 ↗(On Diff #171692)

please no const and auto as well, as the type is not obvious.

147 ↗(On Diff #171692)

please no const

149 ↗(On Diff #171692)

you can shorten this condition with isDependentType

155 ↗(On Diff #171692)

please no const

165 ↗(On Diff #171692)

you could remove both temporaries and return directly and then ellide the braces.
If you don't like that please remove the const

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

Please reword that slightly. The narrowing conversions are defined by the standard, we just implement it "partially" or so.

test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
80 ↗(On Diff #171692)

llvm doesn't add a name to the TODOs.

The necessity to use the correct literal might still be there even if the number is in range: the precisions differ. I believe it could have differences on non-integer values what literal you use respectively.

102 ↗(On Diff #171692)

could you please add tests with integer arithmetic?

E.g. this:

short n1, n2;
float result = n1 + n2; // 'n1 + n2' is of type 'int' because of integer rules
gchatelet updated this revision to Diff 171826.Oct 30 2018, 3:24 PM
gchatelet marked 12 inline comments as done.
  • Address comments
gchatelet marked an inline comment as done.Oct 30 2018, 3:25 PM
gchatelet added inline comments.
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
42 ↗(On Diff #171692)

Sounds good, let's fix this in a follow-up patch.

92 ↗(On Diff #171692)

It's a leftover, I've reworded and changed the code as well.

149 ↗(On Diff #171692)

I tried using the following but it crashes when I run clang tidy other our codebase:

if (LhsType->isDependentType() || RhsType->isDependentType())
    return false;

Maybe I misunderstood what you suggested?

165 ↗(On Diff #171692)

I have to keep both variables or the first one might shortcircuit the second that goes undetected.
e.g. unsigned char c = b ? 1 : -1;

JonasToth added inline comments.Oct 30 2018, 3:48 PM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
149 ↗(On Diff #171692)

You understood correct, but I was wrong. Could you please try LhsType->isInstantiationDependentType(), as this should involve all kind of template surprises.

165 ↗(On Diff #171692)

Indeed, no problem.

courbet added a subscriber: courbet.Nov 5 2018, 1:03 AM
courbet added inline comments.
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
155 ↗(On Diff #171826)

I think it would be clearer to have something like "narrowing conversion from %0 literal to %1"

test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
163 ↗(On Diff #171826)

I think some people would not like to be warned on this (especially for the form if (!returns_int()), because the ! does the cast). What about adding options to the check to disable some forms ?

164 ↗(On Diff #171826)

What about providing a fix for this one :
while (i != 0) {

lebedev.ri added inline comments.
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
164 ↗(On Diff #171826)

Is this actually a narrowing conversion as per the CPPCG?
Conversion to bool is a special case, it's not a truncation, but a x != 0.
I'd think cases like these where bool was pretty clearly meant (if(), while(), for(), ???) should be exempt.

JonasToth added inline comments.Nov 5 2018, 1:50 AM
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
164 ↗(On Diff #171826)

No it is not an narrowing conversion the CPPCG are concerned: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es46-avoid-lossy-narrowing-truncating-arithmetic-conversions

Short:

ES.46: Avoid lossy (narrowing, truncating) arithmetic conversions

Enforcement

A good analyzer can detect all narrowing conversions. However, flagging all narrowing conversions will lead to a lot of false positives. Suggestions:

    - flag all floating-point to integer conversions (maybe only float->char and double->int. Here be dragons! we need data)
    - flag all long->char (I suspect int->char is very common. Here be dragons! we need data)
    - consider narrowing conversions for function arguments especially suspect

We do have a readability-check that is concerned about int -> bool conversions. Because of that I think we could safely ignore the int->bool conversion.
But float -> bool is still suspicious in my opinion. It has the same semantics, but comparing float on equality is dangerous and one could argue it shall be enforced through point 1 (all float-to-int conversions).

gchatelet marked 8 inline comments as done.Nov 5 2018, 2:33 AM
gchatelet added inline comments.
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
164 ↗(On Diff #171826)

@JonasToth Do you have the name of the readability check so I can document the behavior?

JonasToth added inline comments.Nov 5 2018, 3:55 AM
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
164 ↗(On Diff #171826)
gchatelet updated this revision to Diff 172778.Nov 6 2018, 9:21 AM
gchatelet marked 7 inline comments as done.
  • Addressing comments and enhancing diagnostics

I think the check is really close.
If the other reviewers want to take a look at it again, now is a good moment :)

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
178 ↗(On Diff #172778)

I am surprised by following modulo 2 arithmetic and think it's a bit misleading. Writing just module arithmetic is probably better, as module 2 somewhat implies there a only 2 valid values (0, 1).

Is this the int -> unsigned int case path? That seems worth diagnosing too.

224 ↗(On Diff #172778)

Nit: Please move these two comment above the if and make s/conversion/Conversion/.

gchatelet updated this revision to Diff 172904.Nov 7 2018, 12:49 AM
gchatelet marked 2 inline comments as done.
  • Addressing comments
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
178 ↗(On Diff #172778)

Yes, thx for noticing. I updated the comment, I think it's better now.

Indeed this is the int -> unsigned int case path. Warning here would lead to a lot of noise for everybody doing bitwise operations since -1 is a compact way to represent the maximum value. Since the semantic is valid and well defined by the standard I'm unsure it's worth the pain. I'm open to suggestions though.

Maybe a good way to figure out is what would be the correct fix for say unsigned long AllBits = -1;

gchatelet marked 3 inline comments as done.Nov 8 2018, 4:11 AM

Is this good enough to go? @JonasToth

gchatelet retitled this revision from [clang-tidy] Catching narrowing from double to float. to [clang-tidy] Improving narrowing conversions.Nov 8 2018, 8:37 AM
gchatelet edited the summary of this revision. (Show Details)
aaron.ballman added inline comments.Nov 8 2018, 10:52 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
17 ↗(On Diff #172904)

Is this include needed?

60 ↗(On Diff #172904)

Is there a case where you expect the QualType to be invalid? If not, drop the OrNull bit.

69–70 ↗(On Diff #172904)

Spurious parens.

74 ↗(On Diff #172904)

Spurious parens.

86 ↗(On Diff #172904)

Then T should be passed in as a reference type instead of using this assertion.

109 ↗(On Diff #172904)

Perhaps a better approach is to assert this is true and then unilaterally return.

113–114 ↗(On Diff #172904)

This seems like debugging code that should be removed.

133 ↗(On Diff #172904)

Please rename the identifier to not conflict with a type name.

175–176 ↗(On Diff #172904)

Spurious parens.

208–210 ↗(On Diff #172904)

Debugging code. Assert above and unilaterally return.

309 ↗(On Diff #172904)

Assert the above condition and unconditionally return.

JonasToth added inline comments.Nov 8 2018, 2:34 PM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
178 ↗(On Diff #172778)

Comment is fine :)

I though that we have check that diagnoses unsigned i = -1; but I don't find it right now (maybe its still in review or so, i belive it was related to introducing std::numeric_limits<>).
As its well defined and not narrowing and not mentionend by the CPPCG in that section its ok, maybe worth an option in the future?

gchatelet updated this revision to Diff 173343.Nov 9 2018, 8:32 AM
gchatelet marked 13 inline comments as done.
  • Addressing comments, adding Option to disable FP to FP narrowing warnings, handling FP literals.
aaron.ballman added inline comments.Nov 9 2018, 10:05 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
108–113 ↗(On Diff #173343)

This can be further simplified into:

assert(T.isInteger() && "Unexpected builtin type");
return {llvm::APSInt::getMinValue(Context.getTypeSize(&T), T.isUnsignedInteger()),
         llvm::APSInt::getMaxValue(Context.getTypeSize(&T), T.isUnsignedInteger())};
313 ↗(On Diff #173343)

shortciruit -> short circuit

gchatelet updated this revision to Diff 173633.Nov 12 2018, 2:00 AM
gchatelet marked 2 inline comments as done.
  • Address comments + fix hex values display
gchatelet added inline comments.Nov 12 2018, 2:00 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
178 ↗(On Diff #172778)

An Option to warn on these sound like a good idea.
I added TODOs.

Only a few nits from me.

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.