This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Adds modernize-use-bool-literals check.
ClosedPublic

Authored by staronj on Apr 3 2016, 9:26 AM.

Details

Summary

Adds new check to clang-tidy.

Check finds conversions from integer literal to bool.

Diff Detail

Event Timeline

staronj updated this revision to Diff 52504.Apr 3 2016, 9:26 AM
staronj retitled this revision from to [clang-tidy] Adds misc-use-bool-literals check..
staronj updated this object.
Prazek added a subscriber: Prazek.Apr 3 2016, 9:39 AM

Add note in docs/Releasenotes.rst. Besides that it looks good to me, but you still needs the acceptance of Alex (alexfh), who is clang-tidy code owner,

staronj updated this revision to Diff 52507.EditedApr 3 2016, 10:01 AM

Adds note in docs/Releasenotes.rst.

Isn't readability-implicit-bool-cast¶ should catch such issues? If not, I think will be good idea to improve that check instead of introducing new one.

alexfh requested changes to this revision.Apr 4 2016, 4:59 AM
alexfh edited edge metadata.
  1. Looks like modernize would be a better fit for this check, since it targets a pattern common for pre-standard C++ (or C code).
  2. Please clang-format the code.
  3. Please run the check on a large enough codebase (at least, on the whole LLVM) to ensure it doesn't crash and doesn't produce obvious false positives.
This revision now requires changes to proceed.Apr 4 2016, 4:59 AM
Prazek edited edge metadata.Apr 4 2016, 9:17 AM

Isn't readability-implicit-bool-cast¶ should catch such issues? If not, I think will be good idea to improve that check instead of introducing new one.

I wouldn't add this functionality there. I see that readability-implicit-bool-cast aims much different problem.

staronj updated this revision to Diff 52739.Apr 5 2016, 3:34 PM
staronj retitled this revision from [clang-tidy] Adds misc-use-bool-literals check. to [clang-tidy] Adds modernize-use-bool-literals check..
staronj edited edge metadata.
  1. Name changed from misc-use-bool-literals to modernize-use-bool-literals.
  2. Code clang-formatted.
  3. Check ran on LLVM code.
Prazek added a comment.Apr 6 2016, 3:24 AM

So the testing on llvm shows mostly one case - using DEBUG macro like this:

/home/prazek/llvm/lib/Support/APInt.cpp:1656:9: warning: implicitly converting integer literal to bool inside macro, use bool literal instead [modernize-use-bool-literals]

DEBUG(dbgs() << " " << r[i]);
^

/home/prazek/llvm/include/llvm/Support/Debug.h:92:18: note: expanded from macro 'DEBUG'
#define DEBUG(X) DEBUG_WITH_TYPE(DEBUG_TYPE, X)

^

/home/prazek/llvm/include/llvm/Support/Debug.h:69:48: note: expanded from macro 'DEBUG_WITH_TYPE'
#define DEBUG_WITH_TYPE(TYPE, X) do { } while (0)

     ^

Some programers maybe would like to supress this in the case of expressions like while(1), or specially when it is inside macro.
What do you think guys? Should we add some special option like supress-for-while or supress-macro-while?

LegalizeAdulthood added inline comments.
docs/clang-tidy/checks/modernize-use-bool-literals.rst
19

Please ensure the file ends with a newline.

test/clang-tidy/modernize-use-bool-literals.cpp
4

There is more initialization syntax than just assignment. Here are the ones I can think of:

What happens when I write bool foo{1}; or bool foo(1);?

What happens when I write:

class foo {
public:
  foo() : b(0) {}
  foo(int) : b{0} {}
private:
  bool b;
  bool c{0};
  bool d(0);
  bool e = 0;
};

Should we warn an a bool is initialized by a call to a constexpr function that returns an integral type?

Isn't readability-implicit-bool-cast¶ should catch such issues? If not, I think will be good idea to improve that check instead of introducing new one.

I wouldn't add this functionality there. I see that readability-implicit-bool-cast aims much different problem.

We have a lot of casts related checks, so may be will be good idea to introduce dedicated group for such them?

mnbvmar edited edge metadata.Apr 6 2016, 1:01 PM

You could also think whether char literals should be converted to true/false, too. Apart from this (and other people's doubts), everything's fine.

staronj updated this revision to Diff 52843.Apr 6 2016, 1:11 PM
staronj edited edge metadata.
  1. Adds newline at the end of modernize-use-bool-literals.rst file.
  2. Change names in check test for better readability.
  3. Adds some new test cases.
alexfh requested changes to this revision.Apr 7 2016, 11:22 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
40

Use !x.empty() instead of x.size() >= 1.

clang-tidy/modernize/UseBoolLiteralsCheck.h
20

What about the ones explicitly cast to bool? Do we want to warn on them?

test/clang-tidy/modernize-use-bool-literals.cpp
19

Please add CHECK-FIXES to ensure that both the definition of the macro and the code using it are not changed.

This revision now requires changes to proceed.Apr 7 2016, 11:22 AM
staronj updated this revision to Diff 53984.EditedApr 16 2016, 4:03 AM
staronj updated this object.
staronj edited edge metadata.

Check now finds implicit and explicit conversions from integer literal to bool.

Added tests with templates and macros.
Changed function isPreprocessorDependent to use isMacroId function.

staronj marked 5 inline comments as done.Apr 19 2016, 2:41 AM
alexfh requested changes to this revision.Apr 21 2016, 8:17 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
31

Adding two matchers that do almost the same checks is wasteful. You can do the same in a single matcher, which will do twice less work:

implicitCastExpr(has(integerLiteral().bind("literal")),
                 hasImplicitDestinationType(qualType(booleanType())),
                 unless(isInTemplateInstantiation()),
                 hasParent(anyOf(explicitCastExpr().bind("cast"), anything())))

(if anything() here doesn't work, you can probably use expr() or something else).

57

Can you explain, why is it important to note that this happens "inside a macro"?

59

I think, you can write if (Cast) Literal = Cast; and the rest of the code will be much shorter. Also, the isPreprocessorDependent doesn't seem to be needed.

This revision now requires changes to proceed.Apr 21 2016, 8:17 AM
alexfh removed a reviewer: hokein.Apr 21 2016, 8:17 AM
staronj updated this revision to Diff 56395.May 6 2016, 4:06 AM
staronj edited edge metadata.
staronj marked 3 inline comments as done.
staronj added inline comments.
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
57

"Inside a macro" removed.

alexfh accepted this revision.May 6 2016, 8:32 AM
alexfh edited edge metadata.

Looks good!

This revision is now accepted and ready to land.May 6 2016, 8:32 AM
Prazek closed this revision.May 11 2016, 8:40 AM

gg for your first check :)