[clang-tidy] Flag implicit conversion operators.

llvm-svn: 290434

clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp

@@ -22,9 +22,12 @@ namespace google {
 void ExplicitConstructorCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
-  if (getLangOpts().CPlusPlus)
-    Finder->addMatcher(
-        cxxConstructorDecl(unless(isInstantiated())).bind("ctor"), this);
+  if (!getLangOpts().CPlusPlus)
+    return;
+  Finder->addMatcher(cxxConstructorDecl(unless(isInstantiated())).bind("ctor"),
+                     this);
+  Finder->addMatcher(cxxConversionDecl(unless(isExplicit())).bind("conversion"),
+                     this);
 }
 
 // Looks for the token matching the predicate and returns the range of the found
@@ -75,6 +78,17 @@ static bool isStdInitializerList(QualType Type) {
 }
 
 void ExplicitConstructorCheck::check(const MatchFinder::MatchResult &Result) {
+  constexpr char WarningMessage[] =
+      "%0 must be marked explicit to avoid unintentional implicit conversions";
+
+  if (const auto *Conversion =
+      Result.Nodes.getNodeAs<CXXConversionDecl>("conversion")) {
+    SourceLocation Loc = Conversion->getLocation();
+    diag(Loc, WarningMessage)
+        << Conversion << FixItHint::CreateInsertion(Loc, "explicit ");
+    return;
+  }
+
   const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
   // Do not be confused: isExplicit means 'explicit' keyword is present,
   // isImplicit means that it's a compiler-generated constructor.
@@ -101,10 +115,9 @@ void ExplicitConstructorCheck::check(const MatchFinder::MatchResult &Result) {
     else
       ConstructorDescription = "initializer-list";
 
-    DiagnosticBuilder Diag =
-        diag(Ctor->getLocation(),
-             "%0 constructor should not be declared explicit")
-        << ConstructorDescription;
+    auto Diag = diag(Ctor->getLocation(),
+                     "%0 constructor should not be declared explicit")
+                << ConstructorDescription;
     if (ExplicitTokenRange.isValid()) {
       Diag << FixItHint::CreateRemoval(
           CharSourceRange::getCharRange(ExplicitTokenRange));
@@ -119,8 +132,7 @@ void ExplicitConstructorCheck::check(const MatchFinder::MatchResult &Result) {
   bool SingleArgument =
       Ctor->getNumParams() == 1 && !Ctor->getParamDecl(0)->isParameterPack();
   SourceLocation Loc = Ctor->getLocation();
-  diag(Loc,
-       "%0 must be marked explicit to avoid unintentional implicit conversions")
+  diag(Loc, WarningMessage)
       << (SingleArgument
               ? "single-argument constructors"
               : "constructors that are callable with a single argument")

clang-tools-extra/docs/ReleaseNotes.rst

@@ -183,6 +183,10 @@ Improvements to clang-tidy
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-string-cstr.html>`_ check
   now warns about redundant calls to data() too.
 
+- The `google-explicit-constructor
+  <http://clang.llvm.org/extra/clang-tidy/checks/google-explicit-constructor.html>`_ check
+  now warns about conversion operators not marked explicit.
+
 Fixed bugs:
 
 - `modernize-make-unique

clang-tools-extra/docs/clang-tidy/checks/google-explicit-constructor.rst

@@ -4,6 +4,53 @@ google-explicit-constructor
 ===========================
 
 
-Checks that all single-argument constructors are explicit.
+Checks that constructors callable with a single argument and conversion
+operators are marked explicit to avoid the risk of unintentional implicit
+conversions.
+
+Consider this example:
+
+.. code-block:: c++
+
+  struct S {
+    int x;
+    operator bool() const { return true; }
+  };
+
+  bool f() {
+    S a{1};
+    S b{2};
+    return a == b;
+  }
+
+The function will return ``true``, since the objects are implicitly converted to
+``bool`` before comparison, which is unlikely to be the intent.
+
+The check will suggest inserting ``explicit`` before the constructor or
+conversion operator declaration. However, copy and move constructors should not
+be explicit, as well as constructors taking a single ``initializer_list``
+argument.
+
+This code:
+
+.. code-block:: c++
+
+  struct S {
+    S(int a);
+    explicit S(const S&);
+    operator bool() const;
+    ...
+
+will become
+
+.. code-block:: c++
+
+  struct S {
+    explicit S(int a);
+    S(const S&);
+    explicit operator bool() const;
+    ...
+
+
 
 See https://google.github.io/styleguide/cppguide.html#Explicit_Constructors

clang-tools-extra/test/clang-tidy/google-explicit-constructor.cpp

@@ -38,6 +38,7 @@ struct A {
 
   explicit A(void *x) {}
   explicit A(void *x, void *y) {}
+  explicit operator bool() const { return true; }
 
   explicit A(const A& a) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: copy constructor should not be declared explicit [google-explicit-constructor]
@@ -62,6 +63,10 @@ struct B {
   B(const std::initializer_list<unsigned> &list2) {}
   B(std::initializer_list<unsigned> &&list3) {}
 
+  operator bool() const { return true; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'operator bool' must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
+  // CHECK-FIXES: {{^  }}explicit operator bool() const { return true; }
+
   explicit B(::std::initializer_list<double> list4) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: initializer-list constructor should not be declared explicit [google-explicit-constructor]
   // CHECK-FIXES: {{^  }}B(::std::initializer_list<double> list4) {}