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

clang-tidy/modernize/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyModernizeModule
   AvoidBindCheck.cpp
   DeprecatedHeadersCheck.cpp
+  ExplicitOperatorBoolCheck.cpp
   LoopConvertCheck.cpp
   LoopConvertUtils.cpp
   MakeSmartPtrCheck.cpp
   MakeSharedCheck.cpp
   MakeUniqueCheck.cpp
   ModernizeTidyModule.cpp
   PassByValueCheck.cpp
   RawStringLiteralCheck.cpp

clang-tidy/modernize/ExplicitOperatorBoolCheck.h

+//===--- ExplicitOperatorBoolCheck.h - clang-tidy----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_EXPLICIT_OPERATOR_BOOL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_EXPLICIT_OPERATOR_BOOL_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// This check finds implicit operator bool overloads and inserts the explicit
+/// keyword, which is available since C++11.
+/// It also finds implicit operator const void* overloads, which should often be
+/// explicit operator bool overloads.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-explicit-operator-bool.html
+class ExplicitOperatorBoolCheck : public ClangTidyCheck {
+public:
+  ExplicitOperatorBoolCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opt) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const bool WarnOnOperatorVoidPointer;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_EXPLICIT_OPERATOR_BOOL_H

clang-tidy/modernize/ExplicitOperatorBoolCheck.cpp

+//===--- ExplicitOperatorBoolCheck.cpp - clang-tidy------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ExplicitOperatorBoolCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+ExplicitOperatorBoolCheck::ExplicitOperatorBoolCheck(StringRef Name,
+                                                     ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      WarnOnOperatorVoidPointer(Options.get("WarnOnOperatorVoidPointer", 1) !=
+                                0) {}
+
+void ExplicitOperatorBoolCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "WarnOnOperatorVoidPointer", WarnOnOperatorVoidPointer);
+}
+
+void ExplicitOperatorBoolCheck::registerMatchers(MatchFinder *Finder) {
+  // Use of the explicit keyword with operator overloads requires C++11 or
+  // later.
+  if (!getLangOpts().CPlusPlus11)
+    return;
+
+  Finder->addMatcher(
+      cxxConversionDecl(returns(booleanType()), unless(isExplicit()))
+          .bind("operator-bool"),
+      this);
+
+  if (WarnOnOperatorVoidPointer) {
+    Finder->addMatcher(cxxConversionDecl(returns(pointerType(pointee(
+                                             isConstQualified(), voidType()))),
+                                         unless(isExplicit()))
+                           .bind("operator-void-pointer"),
+                       this);
+  }
+}
+
+void ExplicitOperatorBoolCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *MatchedDecl =
+          Result.Nodes.getNodeAs<CXXConversionDecl>("operator-bool")) {
+    diag(MatchedDecl->getLocation(),
+         "operator bool declaration is not explicit")
+        << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "explicit ");
+  } else if (const auto *MatchedDecl =
+                 Result.Nodes.getNodeAs<CXXConversionDecl>(
+                     "operator-void-pointer")) {
+    diag(MatchedDecl->getLocation(),
+         "implicit operator const void* declaration should "
+         "probably be explicit operator bool");
+
+    // FIXME: This tries to change the type and add explicit, but
+    // MatchedDecl->getTypeSpecStartLoc() gets the start of void, not the start
+    // of const, in const void*. We would need something like
+    // getConversionTypeStartLoc().
+    // const auto Type = MatchedDecl->getConversionType().
+    // const auto OldRange = CharSourceRange::getTokenRange(
+    //  MatchedDecl->getTypeSpecStartLoc(),
+    //  MatchedDecl->getTypeSpecStartLoc().getLocWithOffset(Type.getAsString().size()));
+    // diag(MatchedDecl->getLocation(),
+    //  "implicit operator void* declaration should probably be explicit
+    //  operator
+    //  bool")
+    //  << FixItHint::CreateReplacement(OldRange, "bool")
+    //  << FixItHint::CreateInsertion(MatchedDecl->getLocStart(), "explicit ");
+  }
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

clang-tidy/modernize/ModernizeTidyModule.cpp

 //===--- ModernizeTidyModule.cpp - clang-tidy -----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidBindCheck.h"
 #include "DeprecatedHeadersCheck.h"
+#include "ExplicitOperatorBoolCheck.h"
 #include "LoopConvertCheck.h"
 #include "MakeSharedCheck.h"
 #include "MakeUniqueCheck.h"
 #include "PassByValueCheck.h"
 #include "RawStringLiteralCheck.h"
 #include "RedundantVoidArgCheck.h"
 #include "ReplaceAutoPtrCheck.h"
 #include "ShrinkToFitCheck.h"

 
 class ModernizeModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidBindCheck>(
         "modernize-avoid-bind");
     CheckFactories.registerCheck<DeprecatedHeadersCheck>(
         "modernize-deprecated-headers");
+    CheckFactories.registerCheck<ExplicitOperatorBoolCheck>(
+        "modernize-explicit-operator-bool");
     CheckFactories.registerCheck<LoopConvertCheck>("modernize-loop-convert");
     CheckFactories.registerCheck<MakeSharedCheck>("modernize-make-shared");
     CheckFactories.registerCheck<MakeUniqueCheck>("modernize-make-unique");
     CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value");
     CheckFactories.registerCheck<RawStringLiteralCheck>(
         "modernize-raw-string-literal");
     CheckFactories.registerCheck<RedundantVoidArgCheck>(
         "modernize-redundant-void-arg");

docs/ReleaseNotes.rst

- New `modernize-avoid-bind
 
   Finds uses of ``std::bind`` and replaces simple uses with lambdas.
 
 - New `modernize-deprecated-headers
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html>`_ check
 
   Replaces C standard library headers with their C++ alternatives.
 
+- New `modernize-explicit-operator-bool
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-explicit-operator-bool.html>`_ check
+
+  Adds the ``explicit`` keyword to ``operator bool`` overloads.
+  Also finds ``operator const void*`` overloads, which should often be
+  ``explicit operator bool`` overloads.
+
 - New `modernize-make-shared
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html>`_ check
 
   Replaces creation of ``std::shared_ptr`` from new expression with call to ``std::make_shared``.
 
 - New `modernize-raw-string-literal
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-raw-string-literal.html>`_ check
 

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =========================
 
 .. toctree::
    boost-use-to-string
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl50-cpp
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>
    cert-env33-c
    cert-err34-c
    cert-err52-cpp
    cert-err58-cpp
.. toctree::
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-raii
    misc-unused-using-decls
    misc-virtual-near-miss
    modernize-avoid-bind
    modernize-deprecated-headers
+   modernize-explicit-operator-bool
    modernize-loop-convert
    modernize-make-shared
    modernize-make-unique
    modernize-pass-by-value
    modernize-raw-string-literal
    modernize-redundant-void-arg
    modernize-replace-auto-ptr
    modernize-shrink-to-fit

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

+.. title:: clang-tidy - modernize-explicit-operator-bool
+
+modernize-explicit-operator-bool
+================================
+
+This check finds implicit ``operator bool`` overloads and inserts the
+``explicit`` keyword, which is available since C++11.
+
+Without the ``explicit`` keyword, the implicit ``bool`` overload can allow
+objects to be compared accidentally. For instance, even when objects `a` and
+`b` have no ``operator ==`` overloads, an implicit ``operator bool`` would allow
+`a == b` to compile because both `a` and `b` can be implictly converted to
+``bool``.
+
+This check also finds implicit ``operator const void*`` overloads. These were
+often used before C++11 to avoid implicit conversions to ``bool`` when providing
+an ``operator bool`` overload.
+
+To disable the check for ``operator const void*`` overloads, you may set
+The :option:`WarnOnOperatorVoidPointer` option to 1.
+
+.. code-block:: c++
+
+  operator bool () const;
+
+  // becomes
+
+  explicit operator bool () const;

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

+// RUN: %check_clang_tidy %s modernize-explicit-operator-bool %t -- -- -std=c++11
+
+// This should trigger the check:
+class SomethingBad {
+  operator const void *() const {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: implicit operator const void* declaration should probably be explicit operator bool [modernize-explicit-operator-bool]
+    return reinterpret_cast<void *>(something != 0);
+  }
+
+  int something = 0;
+};
+
+class SomethingGood {
+  // Note: Use modernize-explicit-operator-bool to check for implicit operator bool.
+  explicit operator bool() const {
+    return something != 0;
+  }
+
+  int something = 0;
+};
+
+class SomethingGoodExplicitConstVoidPtr {
+  explicit operator const void *() const {
+    return &something;
+  }
+
+  const int something = 0;
+};
+
+class SomethingGoodExplicitNonConstVoidPtr {
+  explicit operator void *() {
+    return &something;
+  }
+
+  int something = 0;
+};
+
+class SomethingGoodNonConstVoidPtr {
+  // A non-const void* is unlikely to to be meant as operator bool before C++11
+  // let us use explicit.
+  operator void *() {
+    return &something;
+  }
+
+  int something = 0;
+};

test/clang-tidy/modernize-explicit-operator-bool.cpp

+// RUN: %check_clang_tidy %s modernize-explicit-operator-bool %t -- -- -std=c++11
+
+// This should trigger the check:
+class SomethingBad {
+  operator bool() const {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator bool declaration is not explicit [modernize-explicit-operator-bool]
+    return something != 0;
+  }
+
+  int something = 0;
+};
+
+class SomethingGood {
+  explicit operator bool() const {
+    return something != 0;
+  }
+
+  int something = 0;
+};