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

[llvmbot]

Bugzilla Link	41199
Version	unspecified
OS	Linux
Attachments	Example cpp file to reproduce the error
Reporter	LLVM Bugzilla Contributor
CC	@chfast,@EugeneZelenko

Extended Description

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

[llvmbot]

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.

[chfast]

How about != 0?

[llvmbot]

'!= 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

llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp

Line 98 in bab1a17

Context->isCheckEnabled("modernize-use-default-member-init")),

for example.

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Assign the issue to you.
2. Fix the issue locally.
3. Run the test suite locally.
   3.1) Remember that the subdirectories under test/ create fine-grained testing targets, so you can
   e.g. use make check-clang-ast to only run Clang's AST tests.
4. Create a git commit
5. Run git clang-format HEAD~1 to format your changes.
6. Submit the patch to Phabricator.
   6.1) Detailed instructions can be found here

For more instructions on how to submit a patch to LLVM, see our documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment on this Github issue.

@llvm/issue-subscribers-good-first-issue