readability-implicit-bool-conversion suggestion causes new readability-uppercase-literal-suffix warning

[Quuxplusone]

Bugzilla Link	PR41199
Status	NEW
Importance	P enhancement
Reported by	Sebastian Buchwald (sebastian.buchwald@posteo.de)
Reported on	2019-03-22 05:49:40 -0700
Last modified on	2020-09-28 03:23:16 -0700
Version	unspecified
Hardware	PC Linux
CC	alexfh@google.com, chfast@gmail.com, djasper@google.com, eugene.zelenko@gmail.com, klimek@google.com, N.James93@hotmail.co.uk, patricknappa@gmail.com
Fixed by commit(s)
Attachments	example.cpp (74 bytes, text/x-c++src)
Blocks
Blocked by
See also

Created attachment 21653
Example cpp  file to reproduce the error

When I run

clang-tidy --checks="readability-*" -fix-errors example.cpp --

on the attached file, I get the following output:

1 warning generated.
example.cpp:3:9: warning: implicit conversion 'unsigned int' -> bool
[readability-implicit-bool-conversion]
    if (s) {
        ^
          != 0u
example.cpp:3:10: note: FIX-IT applied suggested code changes
    if (s) {
         ^
clang-tidy applied 1 of 1 suggested fixes.

However, adding the '!= 0u' causes a new warning, when I re-run the checks:

clang-tidy --checks="readability-*" example.cpp --

1 warning generated.
example.cpp:3:14: warning: integer literal has suffix 'u', which is not
uppercase [readability-uppercase-literal-suffix]
    if (s != 0u) {
             ^~
              U

It would be better if clang-tidy suggests an uppercase suffix (i.e., '!= 0U')
in the first place.

clang-tidy --version
LLVM (http://llvm.org/):
  LLVM version 8.0.0
  Optimized build.
  Default target: x86_64-unknown-linux-gnu
  Host CPU: skylake

[Quuxplusone]

Attached example.cpp (74 bytes, text/x-c++src): Example cpp file to reproduce the error

[Quuxplusone]

Just working on this now - it's a simple fix.

This makes perfect sense if the setting "readability-uppercase-literal-suffix" is enabled (which it is in this case, due to the glob), as fixing to a lowercase is a violation of this rule.

However should the default behaviour be to also amend with an uppercase suffix, even if the "readability-uppercase-literal-suffix" is not enforced? I know it's common at some places that floating point literals should use a lower case suffix (i.e. they use 1.0f rather than 1.0F), s.t. this automatic change may be alien.

Would like to hear people's opinion on this one - making the suffix be uppercased iff the option is enabled isn't difficult from what I'm seeing.

[Quuxplusone]

How about != 0?

[Quuxplusone]

'!= 0' is not a good compromise as it mixes signed and unsigned comparisons. It is easily possible in the constructor for ImplicitBoolConversionCheck to check the context if readability-uppercase-literal-suffix is enabled. See https://github.com/llvm/llvm-project/blob/bab1a17ad7761ae61e5841c2fb905de59cb8c2da/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp#L98 for example.