In D56593#1393325, @jyknight wrote:

It'd be great to see this somewhat more widely publicized, outside of just the clang community. If libc implementors are aware of the gains and are willing to provide an actually-faster bcmp implementation, it'd be a lot better, than having this optimization that doesn't really optimize anything without users providing their own bcmp implementation.

In D57699#1386639, @lebedev.ri wrote:

I think i'm gonna land this.
It's semantically correct, has better perf characteristics, is a very small change, and is symmetrical with writeAsHex().

Thx for working on this Roman. I'm not the one who designed the YAML IO library so perhaps the author is a better reviewer.
LGTM AFAIC

In D53488#1307834, @aaron.ballman wrote:

LGTM!

• Removed redundant options in regression tests

• Fixing documentation.

Thank you for bearing with me @aaron.ballman.

• Addressing comments

• Added a new option warn about wrong type literals

• Reverting the WarnOnFloatingPointNarrowingConversion option to be on by default

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

• Addressed comments

• State char signess clearly

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

In D53488#1296940, @JonasToth wrote:

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

Thanks for your patch!

• Remove debugging leftover

• Address comments

I'm not entirely convinced here:

• The code is harder to read,
• the formatting pattern and the SmallString size may go out of sync,
• the performance win is not that big.

• Address comments + fix hex values display

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

Thx!

Is this good enough to go? @JonasToth

Nice ! Thx for the patch !

• Addressing comments

• Addressing comments and enhancing diagnostics

Did you run the test suite? We may have wrapping lines in the tests.

• Address comments

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]

• Remove leftover

So I ran llvm/tools/clang/tools/extra/clang-tidy/tool/run-clang-tidy.py
The bare version 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 is you're interested.

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.

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

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

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?

• Join the two parts of the ReleaseNotes update

In D53488#1273986, @JonasToth wrote:

Please add a short notice in the release notes for this change.

• Add documentation to ReleaseNotes

• Update documentation, makes returning code path more explicit.

• Addressing comments

• Adding missing comment

• Addressing comments

Nice ! Thx a lot :)

In D53371#1267732, @lebedev.ri wrote:

How do you plan to deal with opcodes that crash (assertion failure) the llvm as a whole?
I.e. not when the snippet crashes, but the generation of the snippet itself crashes.
I *believe* there are some cases.

• Addressing comments

• Fix variable name

• Revert files from other commit to previous state

• Remove dead code and fix comment