search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.69k stars 11.86k forks source link

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

Closed llvmbot closed 2 months ago

llvmbot commented 5 years ago
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 commented 4 years ago

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

chfast commented 4 years ago

How about != 0?

llvmbot commented 5 years ago

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.

llvmbot commented 1 year ago

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