Page MenuHomePhabricator

[clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.
Needs RevisionPublic

Authored by pnappa on Mar 26 2019, 6:33 PM.

Details

Summary

Fix for https://bugs.llvm.org/show_bug.cgi?id=41199#c1

Previously, if a user implicitly cast to a bool (say, in a conditional statement), the fix would be to add an explicit comparison. So, for a floating point implicit case to bool, from if (f), the synthesised code would be if (f != 0.0f).

Even if the flag "readability-uppercase-literal-suffix" was enabled, the synthesised suffix would be lowercase. This commit changes that, such that if that flag is enabled when "readability-implicit-bool-conversion" is enabled, the synthesised suffix is uppercase.

A non-covered case is when "modernize-use-default-member-init" is enabled, lower-case suffixes may still be synthesised (I think, based off the code). Any RFC whether this should be made a different issue or whether or not this behaviour should be added in?

Intended as my first commit to the llvm-project.

Diff Detail

Event Timeline

pnappa created this revision.Mar 26 2019, 6:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2019, 6:33 PM

Please always upload all patches with full context (-U99999)
I'm not sure why we want this? What is wrong with simply applying clang-tidy twice?

aaron.ballman added a subscriber: alexfh.

Intended as my first commit to the llvm-project.

Welcome! Thank you for working on this!

Please always upload all patches with full context (-U99999)
I'm not sure why we want this? What is wrong with simply applying clang-tidy twice?

It doubles the execution time of checking a large project (which may be unacceptably slow), and there's no guarantee that twice will be enough in the general case (one set of fixes may trigger a different check's diagnostics, which may get fixed and trigger another check's diagnostics, etc).

However, it seems like we can run into this sort of interaction in many different checks and in many different ways, and I am not certain that we should try to tackle the maintenance burden of dealing with those interactions ad hoc like this. That said, I'm not certain of a more principled solution either. Adding @alexfh to see if he has put any thought into this area and has ideas.

We normally add something to the documentation about the checker and/or the release notes to say what had changed

clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
48

LLVM normally doesn't put braces on small lines

221

your don't really need the else here because you return above it

alexfh added a comment.Apr 1 2019, 8:24 AM

I'm not sure why we want this? What is wrong with simply applying clang-tidy twice?

It doubles the execution time of checking a large project (which may be unacceptably slow), and there's no guarantee that twice will be enough in the general case (one set of fixes may trigger a different check's diagnostics, which may get fixed and trigger another check's diagnostics, etc).

Yeah, the situation when one automated fix generates another warning is not a nice user experience. It's not only about clang-tidy run time, it's also about annoying users and making their workflow less efficient. Thus it's better to generate clang-tidy-clean code in fixes, where possible and not particularly difficult to implement.

As for the readability-uppercase-literal-suffix check (, I wonder whether clang-format could do this? Non-whitespace changes were historically not desired in clang-format, but there are a number of features there that generate non-whitespace changes (e.g. comment formatting, string literal splitting, #include sorting, macro formatting). This one seems to be local and quite safe. Maybe propose this feature to clang-format?

If the clang-format feature is viable, clang-tidy can already apply clang-format formatting. If not, I'm also not opposed to the new option for readability-implicit-bool-conversion (but maybe it should be a global option like StrictMode)?

alexfh added inline comments.Apr 1 2019, 8:26 AM
clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
48

Maybe use a conditional operator instead?

return Type->isUnsignedIntegerType() ? (UppercaseSuffix ? "0U" : "0u") : "0";
alexfh added inline comments.Apr 1 2019, 9:43 AM
clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
270

Relying on enabled checks for this sort of a behavior is undesired. This sort of an implicit dependency will lead to problems, if, for example, a user wants to apply fixes check-by-check. A separate option (local for this check or better a global one - like StrictMode) would be better.

alexfh requested changes to this revision.Apr 5 2019, 5:50 AM
This revision now requires changes to proceed.Apr 5 2019, 5:50 AM
pnappa updated this revision to Diff 196795.Apr 25 2019, 10:11 PM

Apologies for taking a while, I'm afraid I hadn't had a moment of spare time over the past few weeks.

I believe I've applied the requested changes, bar one that I'm unsure about:

@alexfh to be sure, would adding an Option to "readability-implicit-bool-conversion" called say, "EnforceUppercase", suffice? With that option enabled the synthesis of the explicit comparison would be uppercase.

I'll make the appropriate changes to documentation when I get everything finalised.

pnappa marked 3 inline comments as done.Apr 25 2019, 10:12 PM
lebedev.ri requested changes to this revision.Jul 10 2019, 2:45 PM
lebedev.ri added inline comments.
clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
270

Looks like this wasn't resolved - it should still be a config option, not hardcoded.

This revision now requires changes to proceed.Jul 10 2019, 2:45 PM