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

[• murrayc]

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.

[Eugene.Zelenko]

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.

[etienneb]

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

30 ↗

(On Diff #59196)

nit: const auto *MatchedDecl
and below...

37 ↗

(On Diff #59196)

nit: no { }

37 ↗

(On Diff #59196)

should be part of the matcher.

docs/clang-tidy/checks/list.rst

6–7

why removing this line?

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

12

operator = --> operator == ???

[• murrayc]

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.)

[• murrayc]

In D20857#446101, @etienneb wrote:

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.

[etienneb]

In D20857#446654, @murrayc wrote:

In D20857#446101, @etienneb wrote:

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

20 ↗

(On Diff #59346)

this matcher exists? *voidType*

Matches type void.

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

21 ↗

(On Diff #59346)

nit: indent + 2

31 ↗

(On Diff #59346)

indentation is wrong.
Run clang-format over it

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

35 ↗

(On Diff #59346)

pointer lean to right:

const auto* MatchedDecl
  -->
const auto *MatchedDecl

[• murrayc]

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

[• murrayc]

In D20857#446732, @etienneb wrote:

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

Thanks. Am I being asked to combine the checks?

[etienneb]

In D20857#446784, @murrayc wrote:

In D20857#446732, @etienneb wrote:

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 ↗

(On Diff #59367)

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

40 ↗

(On Diff #59367)

The FIXME only apply to the fixtit statements.

<< FIxItHint(...)

you can still output a diag message.

[• murrayc]

clang-tidy/modernize/OperatorVoidPointerCheck.cpp

27 ↗

(On Diff #59367)

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 ↗

(On Diff #59367)

Yes. I do that in the previous lines.

[aaron.ballman]

In D20857#446784, @murrayc wrote:

In D20857#446732, @etienneb wrote:

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]

test/clang-tidy/modernize-operator-void-pointer.cpp

39 ↗

(On Diff #59367)

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

[Eugene.Zelenko]

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

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

13

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

[• murrayc]

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

[alexfh]

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

7

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

15

nit: Please insert a space after //

[• murrayc]

In D20857#449080, @alexfh wrote:

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

7

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]

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

[• murrayc]

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

[alexfh]

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

clang-tidy/modernize/ExplicitOperatorBoolCheck.cpp

38

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"))));

55

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.

64

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.

[• murrayc]

clang-tidy/modernize/ExplicitOperatorBoolCheck.cpp

38

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*.)