Page MenuHomePhabricator

[clang-tidy] Add modernize-explicit-operator-bool check.
Needs RevisionPublic

Authored by murrayc on Jun 1 2016, 4:11 AM.

Details

Summary

Because implicit operator bool overloads allow accidental comparisons,
such as a == b, via the implicit conversion to bool.

[clang-tidy] Add modernize-operator-void-pointer.

Because an operator void* overload is usually to avoid implicit operator bool,
but C++11 has explicit operator bool.

NOTE: There are some FIXMEs in the code where I think it could be improved. It would be great if someone could suggest existing API to use there.

Diff Detail

Event Timeline

murrayc updated this revision to Diff 59196.Jun 1 2016, 4:11 AM
murrayc retitled this revision from to [clang-tidy] Add modernize-explicit-operator-bool check..
murrayc updated this object.

Please mention these checks in docs/ReleaseNotes.rst (in alphabetical order).

docs/clang-tidy/checks/modernize-explicit-operator-bool.rst
7

Please highlight operator bool, explicit, operator =, bool, etc with ``. Same for other file.

Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: cfe-commits.
etienneb edited reviewers, added: etienneb; removed: etienne.bergeron.Jun 1 2016, 12:57 PM
etienneb edited edge metadata.Jun 1 2016, 1:11 PM

I wonder if these two checks should not be merge in one checker.

modernize-explicit-conversion-operator
clang-tidy/modernize/ExplicitOperatorBoolCheck.cpp
22

As you state later "available since C++11",
you should check for the current language.

Example from other checkers:

if (!getLangOpts().CPlusPlus)
  return;
37

This check should be part of the matcher above.
see: booleanType

Matches type bool.

Given
 struct S { bool func(); };
functionDecl(returns(booleanType()))
  matches "bool func();"
clang-tidy/modernize/OperatorVoidPointerCheck.cpp
31

nit: const auto *MatchedDecl
and below...

38

nit: no { }

38

should be part of the matcher.

docs/clang-tidy/checks/list.rst
7

why removing this line?

docs/clang-tidy/checks/modernize-explicit-operator-bool.rst
12

operator = --> operator == ???

murrayc updated this revision to Diff 59346.Jun 2 2016, 1:05 AM
murrayc edited edge metadata.

These are the commits amended with the suggested changes.
Many thanks for the suggestions.

(I'm not quite sure what arcanist will do, so hopefully this will not create a mess.)

I wonder if these two checks should not be merge in one checker.

Personally I find it cleaner to keep them separate, but I would be happy to combine them if that's wanted. I guessed that it would be easier to accept the explicit bool check than the operator void pointer check, and didn't want to make acceptance harder. I can also imagine someone wanting to disable one but not the other.

I wonder if these two checks should not be merge in one checker.

Personally I find it cleaner to keep them separate, but I would be happy to combine them if that's wanted. I guessed that it would be easier to accept the explicit bool check than the operator void pointer check, and didn't want to make acceptance harder. I can also imagine someone wanting to disable one but not the other.

Enabling/disabling can be done with options (see SizeofExpressionCheck).

WarnOnSizeOfConstant(Options.get("WarnOnSizeOfConstant", 1) != 0),
WarnOnSizeOfThis(Options.get("WarnOnSizeOfThis", 1) != 0),
WarnOnSizeOfCompareToConstant(
    Options.get("WarnOnSizeOfCompareToConstant", 1) != 0) {}
clang-tidy/modernize/OperatorVoidPointerCheck.cpp
21

this matcher exists? *voidType*

Matches type void.

Given
 struct S { void func(); };
functionDecl(returns(voidType()))
  matches "void func();"
22

nit: indent + 2

32

indentation is wrong.
Run clang-format over it

% clang-format -style=file <your-file> -i

36

pointer lean to right:

const auto* MatchedDecl
  -->
const auto *MatchedDecl
murrayc updated this revision to Diff 59367.Jun 2 2016, 5:20 AM

With suggested changes. Ran clang-format (LLVM style). Used voidType() matcher.

Enabling/disabling can be done with options (see SizeofExpressionCheck).

Thanks. Am I being asked to combine the checks?

Enabling/disabling can be done with options (see SizeofExpressionCheck).

Thanks. Am I being asked to combine the checks?

I'll let alexfh@ take decision. He will maintain this code.
It's a matter of preference, and not a blocker to me.

I can still help you to make it conform to the coding style.

clang-tidy/modernize/OperatorVoidPointerCheck.cpp
27

I'm curious, why: isConstQualified() ?
I'm probably missing something.

40

The FIXME only apply to the fixtit statements.

<< FIxItHint(...)

you can still output a diag message.

murrayc added inline comments.Jun 2 2016, 5:47 AM
clang-tidy/modernize/OperatorVoidPointerCheck.cpp
27

This is a workaround for the lack of explicit operator bool before C++:

operator const void*() const;

but this would be unlikely to be meant the same way, partly because the constness would be awkward:

operator void*() const;
40

Yes. I do that in the previous lines.

aaron.ballman edited edge metadata.Jun 2 2016, 6:08 AM

Enabling/disabling can be done with options (see SizeofExpressionCheck).

Thanks. Am I being asked to combine the checks?

I would prefer they be combined as well given how closely related the two checks are.

etienneb added inline comments.Jun 2 2016, 7:22 AM
test/clang-tidy/modernize-operator-void-pointer.cpp
39

You know you can add test for C++11 specifically.

readability-redundant-string-cstr.cpp:// RUN: %check_clang_tidy %s readability-redundant-string-cstr %t -- -- -std=c++11

I think single check will be better from user's point of view.

docs/clang-tidy/checks/modernize-explicit-operator-bool.rst
12

a == b should not be highlighted, or highlighted with (as not language constructs). If highlighted with , same should be done with a, b.

murrayc updated this revision to Diff 59537.Jun 3 2016, 5:41 AM
murrayc edited edge metadata.

Combined into one check. Also specifies C++11 for the test.

alexfh edited edge metadata.Jun 3 2016, 6:35 PM

Looks like a useful check to have. I'm not sure though, that it has anything to do with "modernize". I'd suggest adding a new "bugprone" module (should be added by http://reviews.llvm.org/D18821, hopefully soon) and moving the check there.

test/clang-tidy/modernize-explicit-operator-bool-void-pointer.cpp
6 ↗(On Diff #59537)

From the first glance, this doesn't look like an easy mistake to make. Have you actually seen this pattern in real code?

14 ↗(On Diff #59537)

nit: Please insert a space after //

Looks like a useful check to have. I'm not sure though, that it has anything to do with "modernize". I'd suggest adding a new "bugprone" module (should be added by http://reviews.llvm.org/D18821, hopefully soon) and moving the check there.

Fair enough. My logic is that this is a problem that can only be fixed properly in C++11 and that the best/correct/common way to do this has changed from C++98 to C++11. It's not just a nice use of new syntax (such as auto), it's also fixes bugs, but it's still use of new syntax.

test/clang-tidy/modernize-explicit-operator-bool-void-pointer.cpp
6 ↗(On Diff #59537)

Yes, in glibmm and gtkmm, which I maintain. This commit is from me though the idea wasn't:
https://git.gnome.org/browse/gtkmm/commit/gtk/src/treerowreference.hg?id=c182608593e2d4799f523580a0532fbc68d296b2

We later used a typedef to make that clearer:
https://git.gnome.org/browse/gtkmm/commit/gtk/src/treerowreference.hg?id=7dff74cca47827d6e34bc8f239674bf044ddedaa

There's lots of mention of this in StackOverflow, though not always so clearly. For instance: https://stackoverflow.com/questions/9134888/is-using-void-instead-of-bool-an-advisable-practice

murrayc marked an inline comment as done.
murrayc edited edge metadata.

Same as previous patch, but with a tiny suggested whitespace corretion.

I'd still be perfectly happy if just the simple check for implicit operator bool was accepted.

alexfh requested changes to this revision.Jul 7 2016, 7:04 AM
alexfh edited edge metadata.

Sorry for the delay. Feel free to ping me earlier.

clang-tidy/modernize/ExplicitOperatorBoolCheck.cpp
39

Please merge these two matchers to avoid repeated work. Something along the lines of:

cxxConversionDecl(unless(isExplicit()),
  anyOf(cxxConversionDecl(returns(booleanType())).bind("operator-bool"),
    cxxConversionDecl(returns(pointerType(pointee(isConstQualified(), voidType()))).bind("operator-void-pointer"))));
56

The message should say a bit more on why it is bad, e.g.: "implicit operator bool allows erroneous comparisons; consider marking operator bool 'explicit'". Or in a more prescriptive way: "operator bool should be marked 'explicit' to avoid erroneous comparisons". Feel free to suggest a better option.

65

Looks like you'll need to re-lex the range of the declaration and find the location of tokens const, void, * between operator and (. See clang-tidy/modernize/UseOverrideCheck.cpp for an example.

This revision now requires changes to proceed.Jul 7 2016, 7:04 AM
Prazek edited edge metadata.Jul 11 2016, 3:25 PM
Prazek added a project: Restricted Project.
murrayc added inline comments.Jul 22 2016, 2:38 AM
clang-tidy/modernize/ExplicitOperatorBoolCheck.cpp
39

It seems that I can't pass cxxConversionDecls to anyOf(). I can pass anyOf to cxxConversionDecl, but then I don't see how to bind() to the two kinds of declaration separately:

Finder->addMatcher(

cxxConversionDecl(
    anyOf(returns(booleanType()),
          returns(pointerType(pointee(isConstQualified(), voidType())))),
    unless(isExplicit())),
this);

I also wonder if I'll be able to combine them while keeping the check for WarnOnOperatorVoidPointer, to avoid unnecessary work when that is disabled. (And I still wonder if you even want the check for operator void*.)