[clang-tidy] Improving narrowing conversions

[gchatelet]

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).

[gchatelet]

In 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 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?

[hokein]

In D53488#1275750, @gchatelet wrote:

In 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 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).

[gchatelet]

In D53488#1275786, @hokein wrote:

In D53488#1275750, @gchatelet wrote:

In 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 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)

[JonasToth]

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]

• Add more checks, disable bool implicit casts warning, enable ternary operator warnings.

[gchatelet]

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]

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]

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

[JonasToth]

docs/ReleaseNotes.rst

77 ↗

(On Diff #171692)

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

[Szelethus]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

14 ↗

(On Diff #171692)

Is APInt used anywhere?

[gchatelet]

• Remove leftover

[gchatelet]

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]

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]

• Address comments

[gchatelet]

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]

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]

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]

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]

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]

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]

test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

164 ↗

(On Diff #171826)

Should be https://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html

[gchatelet]

• Addressing comments and enhancing diagnostics

[JonasToth]

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]

• 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]

Is this good enough to go? @JonasToth

[aaron.ballman]

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]

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]

• Addressing comments, adding Option to disable FP to FP narrowing warnings, handling FP literals.

[aaron.ballman]

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]

• Address comments + fix hex values display

[gchatelet]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

178 ↗

(On Diff #172778)

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

[JonasToth]

Only a few nits from me.

[JonasToth]

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]

• Address comments

[gchatelet]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

299 ↗

(On Diff #173633)

Good catch

[gchatelet]

• Remove debugging leftover

[JonasToth]

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

Thanks for your patch!

[gchatelet]

In D53488#1296940, @JonasToth wrote:

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]

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]

• State char signess clearly

[gchatelet]

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]

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]

• Addressed comments

[gchatelet]

test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

79–81 ↗

(On Diff #173844)

Done. Plus added som tests.

[aaron.ballman]

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]

• Refactored the code to make it simpler, added more tests

[gchatelet]

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]

• Reverting the WarnOnFloatingPointNarrowingConversion option to be on by default

[aaron.ballman]

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]

• Added a new option warn about wrong type literals

[gchatelet]

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]

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]

• Addressing comments

[gchatelet]

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]

• Fixing documentation.

[aaron.ballman]

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]

• Removed redundant options in regression tests

[aaron.ballman]

LGTM!

[gchatelet]

In D53488#1307834, @aaron.ballman wrote:

LGTM!

Thx ! \O/

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h

 /// Checks for narrowing conversions, e.g:
 ///   int i = 0;
 ///   i += 0.1;
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.html
 class NarrowingConversionsCheck : public ClangTidyCheck {
 public:
-  NarrowingConversionsCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  NarrowingConversionsCheck(StringRef Name, ClangTidyContext *Context);
+
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void diagNarrowType(SourceLocation SourceLoc, const Expr &Lhs,
+                      const Expr &Rhs);
+
+  void diagNarrowTypeToSignedInt(SourceLocation SourceLoc, const Expr &Lhs,
+                                 const Expr &Rhs);
+
+  void diagNarrowIntegerConstant(SourceLocation SourceLoc, const Expr &Lhs,
+                                 const Expr &Rhs, const llvm::APSInt &Value);
+
+  void diagNarrowIntegerConstantToSignedInt(SourceLocation SourceLoc,
+                                            const Expr &Lhs, const Expr &Rhs,
+                                            const llvm::APSInt &Value,
+                                            const uint64_t HexBits);
+
+  void diagNarrowConstant(SourceLocation SourceLoc, const Expr &Lhs,
+                          const Expr &Rhs);
+
+  void diagConstantCast(SourceLocation SourceLoc, const Expr &Lhs,
+                        const Expr &Rhs);
+
+  void diagNarrowTypeOrConstant(const ASTContext &Context,
+                                SourceLocation SourceLoc, const Expr &Lhs,
+                                const Expr &Rhs);
+
+  void handleIntegralCast(const ASTContext &Context, SourceLocation SourceLoc,
+                          const Expr &Lhs, const Expr &Rhs);
+
+  void handleIntegralToBoolean(const ASTContext &Context,
+                               SourceLocation SourceLoc, const Expr &Lhs,
+                               const Expr &Rhs);
+
+  void handleIntegralToFloating(const ASTContext &Context,
+                                SourceLocation SourceLoc, const Expr &Lhs,
+                                const Expr &Rhs);
+
+  void handleFloatingToIntegral(const ASTContext &Context,
+                                SourceLocation SourceLoc, const Expr &Lhs,
+                                const Expr &Rhs);
+
+  void handleFloatingToBoolean(const ASTContext &Context,
+                               SourceLocation SourceLoc, const Expr &Lhs,
+                               const Expr &Rhs);
+
+  void handleBooleanToSignedIntegral(const ASTContext &Context,
+                                     SourceLocation SourceLoc, const Expr &Lhs,
+                                     const Expr &Rhs);
+
+  void handleFloatingCast(const ASTContext &Context, SourceLocation SourceLoc,
+                          const Expr &Lhs, const Expr &Rhs);
+
+  void handleBinaryOperator(const ASTContext &Context, SourceLocation SourceLoc,
+                            const Expr &Lhs, const Expr &Rhs);
+
+  bool handleConditionalOperator(const ASTContext &Context, const Expr &Lhs,
+                                 const Expr &Rhs);
+
+  void handleImplicitCast(const ASTContext &Context,
+                          const ImplicitCastExpr &Cast);
+
+  void handleBinaryOperator(const ASTContext &Context,
+                            const BinaryOperator &Op);
+
+  const bool WarnOnFloatingPointNarrowingConversion;
+  const bool PedanticMode;
 };
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_NARROWING_CONVERSIONS_H

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

 //===--- NarrowingConversionsCheck.cpp - clang-tidy------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "NarrowingConversionsCheck.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Type.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/APSInt.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+
+#include <cstdint>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
-// FIXME: Check double -> float truncation. Pay attention to casts:
+NarrowingConversionsCheck::NarrowingConversionsCheck(StringRef Name,
+                                                     ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      WarnOnFloatingPointNarrowingConversion(
+          Options.get("WarnOnFloatingPointNarrowingConversion", 1)),
+      PedanticMode(Options.get("PedanticMode", 0)) {}
+
 void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
   // ceil() and floor() are guaranteed to return integers, even though the type
   // is not integral.
-  const auto IsCeilFloorCall = callExpr(callee(functionDecl(
-      hasAnyName("::ceil", "::std::ceil", "::floor", "::std::floor"))));
-
-  const auto IsFloatExpr =
-      expr(hasType(realFloatingPointType()), unless(IsCeilFloorCall));
+  const auto IsCeilFloorCallExpr = expr(callExpr(callee(functionDecl(
+      hasAnyName("::ceil", "::std::ceil", "::floor", "::std::floor")))));
 
-  // casts:
+  // Casts:
   //   i = 0.5;
   //   void f(int); f(0.5);
-  Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(isInteger()),
-                                      hasSourceExpression(IsFloatExpr),
-                                      unless(hasParent(castExpr())),
-                                      unless(isInTemplateInstantiation()))
-                         .bind("cast"),
-                     this);
+  Finder->addMatcher(
+      implicitCastExpr(hasImplicitDestinationType(builtinType()),
+                       hasSourceExpression(hasType(builtinType())),
+                       unless(hasSourceExpression(IsCeilFloorCallExpr)),
 
   // Binary operators:
   //   i += 0.5;
-  Finder->addMatcher(
-      binaryOperator(isAssignmentOperator(),
-                     // The `=` case generates an implicit cast which is covered
-                     // by the previous matcher.
-                     unless(hasOperatorName("=")),
-                     hasLHS(hasType(isInteger())), hasRHS(IsFloatExpr),
-                     unless(isInTemplateInstantiation()))
-          .bind("op"),
+  Finder->addMatcher(binaryOperator(isAssignmentOperator(),
+                                    hasLHS(expr(hasType(builtinType()))),
+                                    hasRHS(expr(hasType(builtinType()))),
+                                    unless(hasRHS(IsCeilFloorCallExpr)),
+                                    unless(isInTemplateInstantiation()),
+                                    // The `=` case generates an implicit cast
+                                    // which is covered by the previous matcher.
+                                    unless(hasOperatorName("=")))
+                         .bind("binary_op"),
                      this);
 }
 
-void NarrowingConversionsCheck::check(const MatchFinder::MatchResult &Result) {
-  if (const auto *Op = Result.Nodes.getNodeAs<BinaryOperator>("op")) {
-    if (Op->getBeginLoc().isMacroID())
-      return;
-    diag(Op->getOperatorLoc(), "narrowing conversion from %0 to %1")
-        << Op->getRHS()->getType() << Op->getLHS()->getType();
+static const BuiltinType *getBuiltinType(const Expr &E) {
+  return E.getType().getCanonicalType().getTypePtr()->getAs<BuiltinType>();
+}
+
+static QualType getUnqualifiedType(const Expr &E) {
+  return E.getType().getUnqualifiedType();
+}
+
+static APValue getConstantExprValue(const ASTContext &Ctx, const Expr &E) {
+  llvm::APSInt IntegerConstant;
+  if (E.isIntegerConstantExpr(IntegerConstant, Ctx))
+    return APValue(IntegerConstant);
+  APValue Constant;
+  if (Ctx.getLangOpts().CPlusPlus && E.isCXX11ConstantExpr(Ctx, &Constant))
+    return Constant;
+  return {};
+}
+
+static bool getIntegerConstantExprValue(const ASTContext &Context,
+                                        const Expr &E, llvm::APSInt &Value) {
+  APValue Constant = getConstantExprValue(Context, E);
+  if (!Constant.isInt())
+    return false;
+  Value = Constant.getInt();
+  return true;
+}
+
+static bool getFloatingConstantExprValue(const ASTContext &Context,
+                                         const Expr &E, llvm::APFloat &Value) {
+  APValue Constant = getConstantExprValue(Context, E);
+  if (!Constant.isFloat())
+    return false;
+  Value = Constant.getFloat();
+  return true;
+}
+
+namespace {
+
+struct IntegerRange {
+  bool Contains(const IntegerRange &From) const {
+    return llvm::APSInt::compareValues(Lower, From.Lower) <= 0 &&
+           llvm::APSInt::compareValues(Upper, From.Upper) >= 0;
+  }
+
+  bool Contains(const llvm::APSInt &Value) const {
+    return llvm::APSInt::compareValues(Lower, Value) <= 0 &&
+           llvm::APSInt::compareValues(Upper, Value) >= 0;
+  }
+
+  llvm::APSInt Lower;
+  llvm::APSInt Upper;
+};
+
+} // namespace
+
+static IntegerRange createFromType(const ASTContext &Context,
+                                   const BuiltinType &T) {
+  if (T.isFloatingPoint()) {
+    unsigned PrecisionBits = llvm::APFloatBase::semanticsPrecision(
+        Context.getFloatTypeSemantics(T.desugar()));
+    // Contrary to two's complement integer, floating point values are
+    // symmetric and have the same number of positive and negative values.
+    // The range of valid integers for a floating point value is:
+    // [-2^PrecisionBits, 2^PrecisionBits]
+
+    // Values are created with PrecisionBits plus two bits:
+    // - One to express the missing negative value of 2's complement
+    //   representation.
+    // - One for the sign.
+    llvm::APSInt UpperValue(PrecisionBits + 2, /*isUnsigned*/ false);
+    UpperValue.setBit(PrecisionBits);
+    llvm::APSInt LowerValue(PrecisionBits + 2, /*isUnsigned*/ false);
+    LowerValue.setBit(PrecisionBits);
+    LowerValue.setSignBit();
+    return {LowerValue, UpperValue};
+  }
+  assert(T.isInteger() && "Unexpected builtin type");
+  uint64_t TypeSize = Context.getTypeSize(&T);
+  bool IsUnsignedInteger = T.isUnsignedInteger();
+  return {llvm::APSInt::getMinValue(TypeSize, IsUnsignedInteger),
+          llvm::APSInt::getMaxValue(TypeSize, IsUnsignedInteger)};
+}
+
+static bool isWideEnoughToHold(const ASTContext &Context,
+                               const BuiltinType &FromType,
+                               const BuiltinType &ToType) {
+  IntegerRange FromIntegerRange = createFromType(Context, FromType);
+  IntegerRange ToIntegerRange = createFromType(Context, ToType);
+  return ToIntegerRange.Contains(FromIntegerRange);
+}
+
+static bool isWideEnoughToHold(const ASTContext &Context,
+                               const llvm::APSInt &IntegerConstant,
+                               const BuiltinType &ToType) {
+  IntegerRange ToIntegerRange = createFromType(Context, ToType);
+  return ToIntegerRange.Contains(IntegerConstant);
+}
+
+static llvm::SmallString<64> getValueAsString(const llvm::APSInt &Value,
+                                              uint64_t HexBits) {
+  llvm::SmallString<64> Str;
+  Value.toString(Str, 10);
+  if (HexBits > 0) {
+    Str.append(" (0x");
+    llvm::SmallString<32> HexValue;
+    Value.toStringUnsigned(HexValue, 16);
+    for (size_t I = HexValue.size(); I < (HexBits / 4); ++I)
+      Str.append("0");
+    Str.append(HexValue);
+    Str.append(")");
+  }
+  return Str;
+}
+
+void NarrowingConversionsCheck::diagNarrowType(SourceLocation SourceLoc,
+                                               const Expr &Lhs,
+                                               const Expr &Rhs) {
+  diag(SourceLoc, "narrowing conversion from %0 to %1")
+      << getUnqualifiedType(Rhs) << getUnqualifiedType(Lhs);
+}
+
+void NarrowingConversionsCheck::diagNarrowTypeToSignedInt(
+    SourceLocation SourceLoc, const Expr &Lhs, const Expr &Rhs) {
+  diag(SourceLoc, "narrowing conversion from %0 to signed type %1 is "
+                  "implementation-defined")
+      << getUnqualifiedType(Rhs) << getUnqualifiedType(Lhs);
+}
+
+void NarrowingConversionsCheck::diagNarrowIntegerConstant(
+    SourceLocation SourceLoc, const Expr &Lhs, const Expr &Rhs,
+    const llvm::APSInt &Value) {
+  diag(SourceLoc,
+       "narrowing conversion from constant value %0 of type %1 to %2")
+      << getValueAsString(Value, /*NoHex*/ 0) << getUnqualifiedType(Rhs)
+      << getUnqualifiedType(Lhs);
+}
+
+void NarrowingConversionsCheck::diagNarrowIntegerConstantToSignedInt(
+    SourceLocation SourceLoc, const Expr &Lhs, const Expr &Rhs,
+    const llvm::APSInt &Value, const uint64_t HexBits) {
+  diag(SourceLoc, "narrowing conversion from constant value %0 of type %1 "
+                  "to signed type %2 is implementation-defined")
+      << getValueAsString(Value, HexBits) << getUnqualifiedType(Rhs)
+      << getUnqualifiedType(Lhs);
+}
+
+void NarrowingConversionsCheck::diagNarrowConstant(SourceLocation SourceLoc,
+                                                   const Expr &Lhs,
+                                                   const Expr &Rhs) {
+  diag(SourceLoc, "narrowing conversion from constant %0 to %1")
+      << getUnqualifiedType(Rhs) << getUnqualifiedType(Lhs);
+}
+
+void NarrowingConversionsCheck::diagConstantCast(SourceLocation SourceLoc,
+                                                 const Expr &Lhs,
+                                                 const Expr &Rhs) {
+  diag(SourceLoc, "constant value should be of type of type %0 instead of %1")
+      << getUnqualifiedType(Lhs) << getUnqualifiedType(Rhs);
+}
+
+void NarrowingConversionsCheck::diagNarrowTypeOrConstant(
+    const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs,
+    const Expr &Rhs) {
+  APValue Constant = getConstantExprValue(Context, Rhs);
+  if (Constant.isInt())
+    return diagNarrowIntegerConstant(SourceLoc, Lhs, Rhs, Constant.getInt());
+  if (Constant.isFloat())
+    return diagNarrowConstant(SourceLoc, Lhs, Rhs);
+  return diagNarrowType(SourceLoc, Lhs, Rhs);
+}
+
+void NarrowingConversionsCheck::handleIntegralCast(const ASTContext &Context,
+                                                   SourceLocation SourceLoc,
+                                                   const Expr &Lhs,
+                                                   const Expr &Rhs) {
+  const BuiltinType *ToType = getBuiltinType(Lhs);
+  // From [conv.integral]p7.3.8:
+  // Conversions to unsigned integer is well defined so no warning is issued.
+  // "The resulting value is the smallest unsigned value equal to the source
+  // value modulo 2^n where n is the number of bits used to represent the
+  // destination type."
+  if (ToType->isUnsignedInteger())
+    return;
+  const BuiltinType *FromType = getBuiltinType(Rhs);
+  llvm::APSInt IntegerConstant;
+  if (getIntegerConstantExprValue(Context, Rhs, IntegerConstant)) {
+    if (!isWideEnoughToHold(Context, IntegerConstant, *ToType))
+      diagNarrowIntegerConstantToSignedInt(SourceLoc, Lhs, Rhs, IntegerConstant,
+                                           Context.getTypeSize(FromType));
+  }
+  if (!isWideEnoughToHold(Context, *FromType, *ToType))
+    diagNarrowTypeToSignedInt(SourceLoc, Lhs, Rhs);
+}
+
+void NarrowingConversionsCheck::handleIntegralToBoolean(
+    const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs,
+    const Expr &Rhs) {
+  // Conversion from Integral to Bool value is well defined.
+
+  // We keep this function (even if it is empty) to make sure that
+  // handleImplicitCast and handleBinaryOperator are symmetric in their behavior
+  // and handle the same cases.
+}
+
+void NarrowingConversionsCheck::handleIntegralToFloating(
+    const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs,
+    const Expr &Rhs) {
+  const BuiltinType *ToType = getBuiltinType(Lhs);
+  llvm::APSInt IntegerConstant;
+  if (getIntegerConstantExprValue(Context, Rhs, IntegerConstant)) {
+    if (!isWideEnoughToHold(Context, IntegerConstant, *ToType))
+      diagNarrowIntegerConstant(SourceLoc, Lhs, Rhs, IntegerConstant);
     return;
   }
-  const auto *Cast = Result.Nodes.getNodeAs<ImplicitCastExpr>("cast");
-  if (Cast->getBeginLoc().isMacroID())
+  const BuiltinType *FromType = getBuiltinType(Rhs);
+  if (!isWideEnoughToHold(Context, *FromType, *ToType))
+    diagNarrowType(SourceLoc, Lhs, Rhs);
+}
+
+void NarrowingConversionsCheck::handleFloatingToIntegral(
+    const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs,
+    const Expr &Rhs) {
+  llvm::APFloat FloatConstant(0.0);
+
+  // We always warn when Rhs is non-constexpr.
+  if (!getFloatingConstantExprValue(Context, Rhs, FloatConstant))
+    return diagNarrowType(SourceLoc, Lhs, Rhs);
+
+  QualType DestType = Lhs.getType();
+  unsigned DestWidth = Context.getIntWidth(DestType);
+  bool DestSigned = DestType->isSignedIntegerOrEnumerationType();
+  llvm::APSInt Result = llvm::APSInt(DestWidth, !DestSigned);
+  bool IsExact = false;
+  bool Overflows = FloatConstant.convertToInteger(
+                       Result, llvm::APFloat::rmTowardZero, &IsExact) &
+                   llvm::APFloat::opInvalidOp;
+  // We warn iff the constant floating point value is not exactly representable.
+  if (Overflows || !IsExact)
+    return diagNarrowConstant(SourceLoc, Lhs, Rhs);
+
+  if (PedanticMode)
+    return diagConstantCast(SourceLoc, Lhs, Rhs);
+}
+
+void NarrowingConversionsCheck::handleFloatingToBoolean(
+    const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs,
+    const Expr &Rhs) {
+  return diagNarrowTypeOrConstant(Context, SourceLoc, Lhs, Rhs);
+}
+
+void NarrowingConversionsCheck::handleBooleanToSignedIntegral(
+    const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs,
+    const Expr &Rhs) {
+  // Conversion from Bool to SignedIntegral value is well defined.
+
+  // We keep this function (even if it is empty) to make sure that
+  // handleImplicitCast and handleBinaryOperator are symmetric in their behavior
+  // and handle the same cases.
+}
+
+void NarrowingConversionsCheck::handleFloatingCast(const ASTContext &Context,
+                                                   SourceLocation SourceLoc,
+                                                   const Expr &Lhs,
+                                                   const Expr &Rhs) {
+  if (WarnOnFloatingPointNarrowingConversion) {
+    const BuiltinType *ToType = getBuiltinType(Lhs);
+    APValue Constant = getConstantExprValue(Context, Rhs);
+    if (Constant.isFloat()) {
+      // From [dcl.init.list]p7.2:
+      // Floating point constant narrowing only takes place when the value is
+      // not within destination range. We convert the value to the destination
+      // type and check if the resulting value is infinity.
+      llvm::APFloat Tmp = Constant.getFloat();
+      bool UnusedLosesInfo;
+      Tmp.convert(Context.getFloatTypeSemantics(ToType->desugar()),
+                  llvm::APFloatBase::rmNearestTiesToEven, &UnusedLosesInfo);
+      if (Tmp.isInfinity())
+        diagNarrowConstant(SourceLoc, Lhs, Rhs);
       return;
-  diag(Cast->getExprLoc(), "narrowing conversion from %0 to %1")
-      << Cast->getSubExpr()->getType() << Cast->getType();
+    }
+    const BuiltinType *FromType = getBuiltinType(Rhs);
+    if (ToType->getKind() < FromType->getKind())
+      diagNarrowType(SourceLoc, Lhs, Rhs);
+  }
+}
+
+void NarrowingConversionsCheck::handleBinaryOperator(const ASTContext &Context,
+                                                     SourceLocation SourceLoc,
+                                                     const Expr &Lhs,
+                                                     const Expr &Rhs) {
+  assert(!Lhs.isInstantiationDependent() && !Rhs.isInstantiationDependent() &&
+         "Dependent types must be check before calling this function");
+  const BuiltinType *LhsType = getBuiltinType(Lhs);
+  const BuiltinType *RhsType = getBuiltinType(Rhs);
+  if (RhsType == nullptr || LhsType == nullptr)
+    return;
+  if (RhsType->getKind() == BuiltinType::Bool && LhsType->isSignedInteger())
+    return handleBooleanToSignedIntegral(Context, SourceLoc, Lhs, Rhs);
+  if (RhsType->isInteger() && LhsType->getKind() == BuiltinType::Bool)
+    return handleIntegralToBoolean(Context, SourceLoc, Lhs, Rhs);
+  if (RhsType->isInteger() && LhsType->isFloatingPoint())
+    return handleIntegralToFloating(Context, SourceLoc, Lhs, Rhs);
+  if (RhsType->isInteger() && LhsType->isInteger())
+    return handleIntegralCast(Context, SourceLoc, Lhs, Rhs);
+  if (RhsType->isFloatingPoint() && LhsType->getKind() == BuiltinType::Bool)
+    return handleFloatingToBoolean(Context, SourceLoc, Lhs, Rhs);
+  if (RhsType->isFloatingPoint() && LhsType->isInteger())
+    return handleFloatingToIntegral(Context, SourceLoc, Lhs, Rhs);
+  if (RhsType->isFloatingPoint() && LhsType->isFloatingPoint())
+    return handleFloatingCast(Context, SourceLoc, Lhs, Rhs);
+}
+
+bool NarrowingConversionsCheck::handleConditionalOperator(
+    const ASTContext &Context, const Expr &Lhs, const Expr &Rhs) {
+  if (const auto *CO = llvm::dyn_cast<ConditionalOperator>(&Rhs)) {
+    // We have an expression like so: `output = cond ? lhs : rhs`
+    // From the point of view of narrowing conversion we treat it as two
+    // expressions `output = lhs` and `output = rhs`.
+    handleBinaryOperator(Context, CO->getLHS()->getExprLoc(), Lhs,
+                         *CO->getLHS());
+    handleBinaryOperator(Context, CO->getRHS()->getExprLoc(), Lhs,
+                         *CO->getRHS());
+    return true;
+  }
+  return false;
+}
+
+void NarrowingConversionsCheck::handleImplicitCast(
+    const ASTContext &Context, const ImplicitCastExpr &Cast) {
+  if (Cast.getExprLoc().isMacroID())
+    return;
+  const Expr &Lhs = Cast;
+  const Expr &Rhs = *Cast.getSubExpr();
+  if (Lhs.isInstantiationDependent() || Rhs.isInstantiationDependent())
+    return;
+  if (handleConditionalOperator(Context, Lhs, Rhs))
+    return;
+  SourceLocation SourceLoc = Lhs.getExprLoc();
+  switch (Cast.getCastKind()) {
+  case CK_BooleanToSignedIntegral:
+    return handleBooleanToSignedIntegral(Context, SourceLoc, Lhs, Rhs);
+  case CK_IntegralToBoolean:
+    return handleIntegralToBoolean(Context, SourceLoc, Lhs, Rhs);
+  case CK_IntegralToFloating:
+    return handleIntegralToFloating(Context, SourceLoc, Lhs, Rhs);
+  case CK_IntegralCast:
+    return handleIntegralCast(Context, SourceLoc, Lhs, Rhs);
+  case CK_FloatingToBoolean:
+    return handleFloatingToBoolean(Context, SourceLoc, Lhs, Rhs);
+  case CK_FloatingToIntegral:
+    return handleFloatingToIntegral(Context, SourceLoc, Lhs, Rhs);
+  case CK_FloatingCast:
+    return handleFloatingCast(Context, SourceLoc, Lhs, Rhs);
+  default:
+    break;
+  }
+}
+
+void NarrowingConversionsCheck::handleBinaryOperator(const ASTContext &Context,
+                                                     const BinaryOperator &Op) {
+  if (Op.getBeginLoc().isMacroID())
+    return;
+  const Expr &Lhs = *Op.getLHS();
+  const Expr &Rhs = *Op.getRHS();
+  if (Lhs.isInstantiationDependent() || Rhs.isInstantiationDependent())
+    return;
+  if (handleConditionalOperator(Context, Lhs, Rhs))
+    return;
+  handleBinaryOperator(Context, Rhs.getBeginLoc(), Lhs, Rhs);
+}
+
+void NarrowingConversionsCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Op = Result.Nodes.getNodeAs<BinaryOperator>("binary_op"))
+    return handleBinaryOperator(*Result.Context, *Op);
+  if (const auto *Cast = Result.Nodes.getNodeAs<ImplicitCastExpr>("cast"))
+    return handleImplicitCast(*Result.Context, *Cast);
+  llvm_unreachable("must be binary operator or cast expression");
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New alias :doc:`hicpp-uppercase-literal-suffix
   :doc:`readability-uppercase-literal-suffix
   <clang-tidy/checks/readability-uppercase-literal-suffix>`
   added.
 
 - The :doc:`readability-redundant-smartptr-get
   <clang-tidy/checks/readability-redundant-smartptr-get>` check does not warn
   about calls inside macros anymore by default.
 
+- The :doc:`cppcoreguidelines-narrowing-conversions
+  <clang-tidy/checks/cppcoreguidelines-narrowing-conversions>` check now
+  detects more 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).
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst

 .. title:: clang-tidy - cppcoreguidelines-narrowing-conversions
 
 cppcoreguidelines-narrowing-conversions
 =======================================
 
 Checks for silent narrowing conversions, e.g: ``int i = 0; i += 0.1;``. While
 the issue is obvious in this former example, it might not be so in the
 following: ``void MyClass::f(double d) { int_member_ += d; }``.
 
 This rule is part of the "Expressions and statements" profile of the C++ Core
 Guidelines, corresponding to rule ES.46. See
 
-https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-narrowing.
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es46-avoid-lossy-narrowing-truncating-arithmetic-conversions.
 
-We enforce only part of the guideline, more specifically, we flag:
- - All floating-point to integer conversions that are not marked by an explicit
-   cast (c-style or ``static_cast``). For example: ``int i = 0; i += 0.1;``,
+We enforce only part of the guideline, more specifically, we flag narrowing conversions from:
+ - an integer to a narrower integer (e.g. ``char`` to ``unsigned char``),
+ - an integer to a narrower floating-point (e.g. ``uint64_t`` to ``float``),
+ - a floating-point to an integer (e.g. ``double`` to ``int``),
+ - a floating-point to a narrower floating-point (e.g. ``double`` to ``float``)
+   if WarnOnFloatingPointNarrowingConversion Option is set.
+
+This check will flag:
+ - All narrowing conversions that are not marked by an explicit cast (c-style or
+   ``static_cast``). For example: ``int i = 0; i += 0.1;``,
    ``void f(int); f(0.1);``,
- - All applications of binary operators where the left-hand-side is an integer
-   and the right-hand-size is a floating-point. For example:
-   ``int i; i+= 0.1;``.
+ - All applications of binary operators with a narrowing conversions.
+   For example: ``int i; i+= 0.1;``.
 
+
+ Options
+ -------
+
+ .. option:: WarnOnFloatingPointNarrowingConversion
+
+    When non-zero, the check will warn on narrowing floating point conversion
+    (e.g. ``double`` to ``float``). `1` by default.
+
+  .. option:: PedanticMode
+
+    When non-zero, the check will warn on assigning a floating point constant
+    to an integer value even if the floating point value is exactly
+    representable in the destination type (e.g. ``int i = 1.0;``).
+    `0` by default.
+
+FAQ
+---
+
+ - What does "narrowing conversion from 'int' to 'float'" mean?
+
+An IEEE754 Floating Point number can represent all integer values in the range
+[-2^PrecisionBits, 2^PrecisionBits] where PrecisionBits is the number of bits in
+the mantissa.
+
+For ``float`` this would be [-2^23, 2^23], where ``int`` can represent values in
+the range [-2^31, 2^31-1].
+
+ - What does "implementation-defined" mean?
+
+You may have encountered messages like "narrowing conversion from 'unsigned int'
+to signed type 'int' is implementation-defined".
+The C/C++ standard does not mandate two’s complement for signed integers, and so
+the compiler is free to define what the semantics are for converting an unsigned
+integer to signed integer. Clang's implementation uses the two’s complement
+format.

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-narrowing-conversions-long-is-32bits.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
+// RUN: -- -- -target x86_64-unknown-linux -m32
+
+static_assert(sizeof(int) * 8 == 32, "int is 32-bits");
+static_assert(sizeof(long) * 8 == 32, "long is 32-bits");
+static_assert(sizeof(long long) * 8 == 64, "long long is 64-bits");
+
+void narrow_integer_to_signed_integer_is_not_ok() {
+  int i;        // i.e. int32_t
+  long l;       // i.e. int32_t
+  long long ll; // i.e. int64_t
+
+  unsigned int ui;        // i.e. uint32_t
+  unsigned long ul;       // i.e. uint32_t
+  unsigned long long ull; // i.e. uint64_t
+
+  i = l;  // int and long are the same type.
+  i = ll; // int64_t does not fit in an int32_t
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  ll = ul;  // uint32_t fits into int64_t
+  ll = ull; // uint64_t does not fit in an int64_t
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'unsigned long long' to signed type 'long long' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-narrowing-conversions-narrowingfloatingpoint-option.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
+// RUN: -- -- -target x86_64-unknown-linux -fsigned-char
+
+namespace floats {
+
+void narrow_constant_floating_point_to_int_not_ok(double d) {
+  int i = 0;
+  i += 0.5;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i += 0.5f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i *= 0.5f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i /= 0.5f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i += (double)0.5f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i += 2.0;
+  i += 2.0f;
+}
+
+double operator"" _double(unsigned long long);
+
+float narrow_double_to_float_return() {
+  return 0.5;
+}
+
+void narrow_double_to_float_not_ok(double d) {
+  float f;
+  f = d;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  f = 15_double;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  f += d;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  f = narrow_double_to_float_return();
+}
+
+void narrow_fp_constants() {
+  float f;
+  f = 0.5; // [dcl.init.list] 7.2 : in-range fp constant to narrower float is not a narrowing.
+
+  f = __builtin_huge_valf();  // max float is not narrowing.
+  f = -__builtin_huge_valf(); // -max float is not narrowing.
+  f = __builtin_inff();       // float infinity is not narrowing.
+  f = __builtin_nanf("0");    // float NaN is not narrowing.
+
+  f = __builtin_huge_val(); // max double is not within-range of float.
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from constant 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  f = -__builtin_huge_val(); // -max double is not within-range of float.
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from constant 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  f = __builtin_inf(); // double infinity is not within-range of float.
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from constant 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  f = __builtin_nan("0"); // double NaN is not narrowing.
+}
+
+} // namespace floats

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-narrowing-conversions-pedanticmode-option.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
+// RUN: -config="{CheckOptions: [ \
+// RUN:   {key: "cppcoreguidelines-narrowing-conversions.PedanticMode", value: 1} \
+// RUN: ]}" \
+// RUN: -- -target x86_64-unknown-linux -fsigned-char
+
+namespace floats {
+
+void triggers_wrong_constant_type_warning(double d) {
+  int i = 0.0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: constant value should be of type of type 'int' instead of 'double' [cppcoreguidelines-narrowing-conversions]
+  i += 2.0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: constant value should be of type of type 'int' instead of 'double' [cppcoreguidelines-narrowing-conversions]
+  i += 2.0f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: constant value should be of type of type 'int' instead of 'float' [cppcoreguidelines-narrowing-conversions]
+}
+
+void triggers_narrowing_warning_when_overflowing() {
+  unsigned short us = 65537.0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: narrowing conversion from constant 'double' to 'unsigned short' [cppcoreguidelines-narrowing-conversions]
+}
+
+} // namespace floats

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-narrowing-conversions-unsigned-char.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
+// RUN: -- -- -target x86_64-unknown-linux -funsigned-char
+
+void narrow_integer_to_unsigned_integer_is_ok() {
+  signed char sc;
+  short s;
+  int i;
+  long l;
+  long long ll;
+
+  char c;
+  unsigned short us;
+  unsigned int ui;
+  unsigned long ul;
+  unsigned long long ull;
+
+  ui = sc;
+  c = s;
+  c = i;
+  c = l;
+  c = ll;
+
+  c = c;
+  c = us;
+  c = ui;
+  c = ul;
+  c = ull;
+}
+
+void narrow_integer_to_signed_integer_is_not_ok() {
+  signed char sc;
+  short s;
+  int i;
+  long l;
+  long long ll;
+
+  char c;
+  unsigned short us;
+  unsigned int ui;
+  unsigned long ul;
+  unsigned long long ull;
+
+  sc = sc;
+  sc = s;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'short' to signed type 'signed char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  sc = i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'int' to signed type 'signed char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  sc = l;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'long' to signed type 'signed char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  sc = ll;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'long long' to signed type 'signed char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+
+  sc = c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'char' to signed type 'signed char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  sc = us;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'unsigned short' to signed type 'signed char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  sc = ui;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'unsigned int' to signed type 'signed char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  sc = ul;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'unsigned long' to signed type 'signed char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  sc = ull;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'unsigned long long' to signed type 'signed char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_constant_to_unsigned_integer_is_ok() {
+  char c1 = -128; // unsigned dst type is well defined.
+  char c2 = 127;  // unsigned dst type is well defined.
+  char c3 = -129; // unsigned dst type is well defined.
+  char c4 = 128;  // unsigned dst type is well defined.
+  unsigned char uc1 = 0;
+  unsigned char uc2 = 255;
+  unsigned char uc3 = -1;  // unsigned dst type is well defined.
+  unsigned char uc4 = 256; // unsigned dst type is well defined.
+  signed char sc = 128;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: narrowing conversion from constant value 128 (0x00000080) of type 'int' to signed type 'signed char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_conditional_operator_contant_to_unsigned_is_ok(bool b) {
+  // conversion to unsigned char type is well defined.
+  char c1 = b ? 1 : 0;
+  char c2 = b ? 1 : 256;
+  char c3 = b ? -1 : 0;
+}

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
+// RUN: -config="{CheckOptions: [ \
+// RUN:   {key: "cppcoreguidelines-narrowing-conversions.WarnOnFloatingPointNarrowingConversion", value: 0}, \
+// RUN: ]}" \
+// RUN: -- -target x86_64-unknown-linux -fsigned-char
 
 float ceil(float);
 namespace std {
 double ceil(double);
 long double floor(long double);
 } // namespace std
 
 namespace floats {
 
 struct ConvertsToFloat {
-  operator float() const { return 0.5; }
+  operator float() const { return 0.5f; }
 };
 
-float operator "" _Pa(unsigned long long);
+float operator"" _float(unsigned long long);
 
-void not_ok(double d) {
+void narrow_fp_to_int_not_ok(double d) {
   int i = 0;
   i = d;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = static_cast<float>(d);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = ConvertsToFloat();
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-  i = 15_Pa;
+  i = 15_float;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-}
-
-void not_ok_binary_ops(double d) {
-  int i = 0;
+  i += d;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += 0.5;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-  i += d;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
-  // We warn on the following even though it's not dangerous because there is no
-  // reason to use a double literal here.
-  // TODO(courbet): Provide an automatic fix.
-  i += 2.0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
-  i += 2.0f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i *= 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i /= 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += (double)0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i += 2.0;
+  i += 2.0f;
+}
+
+double operator"" _double(unsigned long long);
+
+float narrow_double_to_float_return() {
+  return 0.5; // [dcl.init.list] 7.2 : in-range fp constant to narrower float is not a narrowing.
+}
+
+void narrow_double_to_float_ok(double d) {
+  float f;
+  f = d;
+  f = 15_double;
+}
+
+void narrow_fp_constants() {
+  float f;
+  f = 0.5; // [dcl.init.list] 7.2 : in-range fp constant to narrower float is not a narrowing.
+
+  f = __builtin_huge_valf();  // max float is not narrowing.
+  f = -__builtin_huge_valf(); // -max float is not narrowing.
+  f = __builtin_inff();       // float infinity is not narrowing.
+  f = __builtin_nanf("0");    // float NaN is not narrowing.
+
+  f = __builtin_huge_val();  // max double is not within-range of float.
+  f = -__builtin_huge_val(); // -max double is not within-range of float.
+  f = __builtin_inf();       // double infinity is not within-range of float.
+  f = __builtin_nan("0");    // double NaN is not narrowing.
+}
+
+void narrow_double_to_float_not_ok_binary_ops(double d) {
+  float f;
+  f += 0.5;          // [dcl.init.list] 7.2 : in-range fp constant to narrower float is not a narrowing.
+  f += 2.0;          // [dcl.init.list] 7.2 : in-range fp constant to narrower float is not a narrowing.
+  f *= 0.5;          // [dcl.init.list] 7.2 : in-range fp constant to narrower float is not a narrowing.
+  f /= 0.5;          // [dcl.init.list] 7.2 : in-range fp constant to narrower float is not a narrowing.
+  f += (double)0.5f; // [dcl.init.list] 7.2 : in-range fp constant to narrower float is not a narrowing.
+  f += d;            // We do not warn about floating point narrowing by default.
+}
+
+void narrow_fp_constant_to_bool_not_ok() {
+  bool b1 = 1.0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from constant 'double' to 'bool' [cppcoreguidelines-narrowing-conversions]
+  bool b2 = 1.0f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from constant 'float' to 'bool' [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_integer_to_floating() {
+  {
+    long long ll; // 64 bits
+    float f = ll; // doesn't fit in 24 bits
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'long long' to 'float' [cppcoreguidelines-narrowing-conversions]
+    double d = ll; // doesn't fit in 53 bits.
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: narrowing conversion from 'long long' to 'double' [cppcoreguidelines-narrowing-conversions]
+  }
+  {
+    int i;       // 32 bits
+    float f = i; // doesn't fit in 24 bits
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'int' to 'float' [cppcoreguidelines-narrowing-conversions]
+    double d = i; // fits in 53 bits.
+  }
+  {
+    short n1, n2;
+    float f = n1 + n2; // 'n1 + n2' is of type 'int' because of integer rules
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'int' to 'float' [cppcoreguidelines-narrowing-conversions]
+  }
+  {
+    short s;      // 16 bits
+    float f = s;  // fits in 24 bits
+    double d = s; // fits in 53 bits.
+  }
+}
+
+void narrow_integer_to_unsigned_integer_is_ok() {
+  char c;
+  short s;
+  int i;
+  long l;
+  long long ll;
+
+  unsigned char uc;
+  unsigned short us;
+  unsigned int ui;
+  unsigned long ul;
+  unsigned long long ull;
+
+  ui = c;
+  uc = s;
+  uc = i;
+  uc = l;
+  uc = ll;
+
+  uc = uc;
+  uc = us;
+  uc = ui;
+  uc = ul;
+  uc = ull;
+}
+
+void narrow_integer_to_signed_integer_is_not_ok() {
+  char c;
+  short s;
+  int i;
+  long l;
+  long long ll;
+
+  unsigned char uc;
+  unsigned short us;
+  unsigned int ui;
+  unsigned long ul;
+  unsigned long long ull;
+
+  c = c;
+  c = s;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'short' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  c = i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'int' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  c = l;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  c = ll;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+
+  c = uc;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned char' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  c = us;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned short' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  c = ui;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned int' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  c = ul;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned long' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  c = ull;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned long long' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+
+  i = c;
+  i = s;
+  i = i;
+  i = l;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  i = ll;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+
+  i = uc;
+  i = us;
+  i = ui;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  i = ul;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  i = ull;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+
+  ll = c;
+  ll = s;
+  ll = i;
+  ll = l;
+  ll = ll;
+
+  ll = uc;
+  ll = us;
+  ll = ui;
+  ll = ul;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'unsigned long' to signed type 'long long' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  ll = ull;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'unsigned long long' to signed type 'long long' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_constant_to_unsigned_integer_is_ok() {
+  unsigned char uc1 = 0;
+  unsigned char uc2 = 255;
+  unsigned char uc3 = -1;  // unsigned dst type is well defined.
+  unsigned char uc4 = 256; // unsigned dst type is well defined.
+  unsigned short us1 = 0;
+  unsigned short us2 = 65535;
+  unsigned short us3 = -1;    // unsigned dst type is well defined.
+  unsigned short us4 = 65536; // unsigned dst type is well defined.
+}
+
+void narrow_constant_to_signed_integer_is_not_ok() {
+  char c1 = -128;
+  char c2 = 127;
+  char c3 = -129;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from constant value -129 (0xFFFFFF7F) of type 'int' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  char c4 = 128;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from constant value 128 (0x00000080) of type 'int' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+
+  short s1 = -32768;
+  short s2 = 32767;
+  short s3 = -32769;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: narrowing conversion from constant value -32769 (0xFFFF7FFF) of type 'int' to signed type 'short' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  short s4 = 32768;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: narrowing conversion from constant value 32768 (0x00008000) of type 'int' to signed type 'short' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_conditional_operator_contant_to_unsigned_is_ok(bool b) {
+  // conversion to unsigned dst type is well defined.
+  unsigned char c1 = b ? 1 : 0;
+  unsigned char c2 = b ? 1 : 256;
+  unsigned char c3 = b ? -1 : 0;
+}
+
+void narrow_conditional_operator_contant_to_signed_is_not_ok(bool b) {
+  char uc1 = b ? 1 : 0;
+  char uc2 = b ? 1 : 128;
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: narrowing conversion from constant value 128 (0x00000080) of type 'int' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  char uc3 = b ? -129 : 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: narrowing conversion from constant value -129 (0xFFFFFF7F) of type 'int' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  unsigned long long ysize;
+  long long mirror = b ? -1 : ysize - 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: narrowing conversion from constant value 18446744073709551615 (0xFFFFFFFFFFFFFFFF) of type 'unsigned long long' to signed type 'long long' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-2]]:37: warning: narrowing conversion from 'unsigned long long' to signed type 'long long' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_constant_to_floating_point() {
+  float f_ok = 1ULL << 24;              // fits in 24 bits mantissa.
+  float f_not_ok = (1ULL << 24) + 1ULL; // doesn't fit in 24 bits mantissa.
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: narrowing conversion from constant value 16777217 of type 'unsigned long long' to 'float' [cppcoreguidelines-narrowing-conversions]
+  double d_ok = 1ULL << 53;              // fits in 53 bits mantissa.
+  double d_not_ok = (1ULL << 53) + 1ULL; // doesn't fit in 53 bits mantissa.
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: narrowing conversion from constant value 9007199254740993 of type 'unsigned long long' to 'double' [cppcoreguidelines-narrowing-conversions]
+}
+
+void casting_integer_to_bool_is_ok() {
+  int i;
+  while (i) {
+  }
+  for (; i;) {
+  }
+  if (i) {
+  }
+}
+
+void casting_float_to_bool_is_not_ok() {
+  float f;
+  while (f) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'float' to 'bool' [cppcoreguidelines-narrowing-conversions]
+  }
+  for (; f;) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'float' to 'bool' [cppcoreguidelines-narrowing-conversions]
+  }
+  if (f) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'bool' [cppcoreguidelines-narrowing-conversions]
+  }
+}
+
+void legitimate_comparison_do_not_warn(unsigned long long size) {
+  for (int i = 0; i < size; ++i) {
+  }
 }
 
 void ok(double d) {
   int i = 0;
   i = 1;
   i = static_cast<int>(0.5);
   i = static_cast<int>(d);
   i = std::ceil(0.5);

 // We're bailing out in templates and macros.
 template <typename T1, typename T2>
 void f(T1 one, T2 two) {
   one += two;
 }
 
 void template_context() {
   f(1, 2);
+  f(1, .5f);
   f(1, .5);
+  f(1, .5l);
 }
 
 #define DERP(i, j) (i += j)
 
 void macro_context() {
   int i = 0;
   DERP(i, 2);
+  DERP(i, .5f);
   DERP(i, .5);
+  DERP(i, .5l);
 }
 
-}  // namespace floats
+} // namespace floats