[clang-tidy] cppcoreguidelines-explicit-constructor-and-conversion: new alias

[mgartmann]

Summary

Since the google-explicit-constructor check implements rules C.46 and C.164 from the CppCoreGuidelines, alias cppcoreguidelines-explicit-constructor-and-conversion was created.

C.46 proposes excluding any non-explicit single-argument constructors on a positive list. Such a whitelist option was added to google-explicit-constructor check.

[mgartmann]

Added ConstructorWhitelist option to the google-explicit-constructor check.

[mgartmann]

Dear reviewers,

Since this work was conducted as part of a Bachelor's thesis, which has to be
handed in on the 18th of June, 2021, we wanted to add a friendly ping to this
patch.

It would make us, the thesis team, proud to state that some of our work was
accepted and included into the LLVM project.

Therefore, we wanted to ask if we can further improve anything in this patch
for it to get a LGTM?

Thanks a lot in advance!
Yours faithfully,
Fabian & Marco

[aaron.ballman]

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

22

23–24

It's unfortunate that this is parsing the list of names each time -- I think we should parse the string list one time and pass a container in to this function rather than doing the split every time we encounter a constructor.

102

Unintended formatting change?

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

28

The community has recently been switching away from "whitelist" and "blacklist" terminology, so I'd recommend changing this to IgnoredConstructors.

Should conversion operators also have an allow list? (The core guideline doesn't mention it, but I'm wondering about code that wants an implicit conversion in a particular case and how they should silence the diagnostic in that case.)

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

11

It looks like there are no tests for conversion operators -- those should be added.

[mgartmann]

• added option to ignore conversion operators
• added tests for new and existing options
• renamed options to Ignore...
• ensured that option's strings only get parsed once

[mgartmann]

@aaron.ballman Thanks a lot for your valuable feedback! I incorporated it accordingly.

Is there anything else that should be improved?

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

102

This formatting change was done by clang-format when git clang-format HEAD~1 was run. Therefore, I assumed that it is now correctly formatted.

Am I wrong?

[aaron.ballman]

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

28–30

Rather than trying to do this by hand, does Decl::print() give you the functionality you need? For example, this will likely not work well for classes defined within a namespace (it may ignore the wrong class due to not checking the namespace). Another thing to consider are templates and how to handle those. e.g.,

struct Foo {
  template <typename Ty>
  operator Ty() const; // How to silence the diagnostic here?
};

Thankfully, specializations can't differ in their explicitness, so you don't have to also worry about:

struct Foo {
  template <typename Ty>
  explicit operator Ty() const; // This one's explicit
};

template <>
Foo::operator int() const; // Thankfully, this inherits the explicit from the primary template.

33

102

I've never used the integrated git clang-format before, but I sort of wonder if it is getting extra context lines and that's why this one changed? I usually do git diff -U0 --no-color HEAD^ | clang-format-diff.py -i -p1 and then I know exactly what range of lines are being touched.

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

74–75

We should probably document other identifiers that contribute to the name of the operator, like namespaces, template ids, etc. Using code examples for anything you think is tricky would be helpful.

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

84 ↗

(On Diff #347970)

Can you also add an example that an explicit conversion operator does not diagnose?

[mgartmann]

• added testcase of explicit operator

[njames93]

Is it worth using regex to ignore some operators:
*::operator bool to ignore all bool conversions etc.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-explicit-constructor-and-conversion.rst

13–15

The page generated from here will automatically redirect to the alias after 5 seconds so any extra documentation in here won't be read.

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

75

double quotes aren't needed in options.

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

1–4 ↗

(On Diff #348178)

Passing an explicitly empty config is unnecessary.

[mgartmann]

• removed empty configs from tests
• moved documentation to Google's check
• extended matchers so that namespaces for classes and conversion operators can be specified
• adjusted documentation and tests

Unfortunately, I was not able to implement functionality to ignore constructors and operates by using Regex in the given time.

[mgartmann]

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

28–30

Thanks for your comment, @aaron.ballman!

I was able to use printQualifiedName to get a Node's qualified name, including its namespace.

Templated operators are not represented by their name visible in the source code (e.g., Ty) in the AST. Instead, their name in the AST is something like type-parameter-0-0. As it is now, templated operators have to be ignored with the latter name, which is also displayed in the check's diagnostic message.
This was described in the documentation accordingly.

I was not able to find a feasible way to enable users to exclude templated operators by their original name (e.g., Ty). Does anyone have an idea how this could be done?

clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===-- CppCoreGuidelinesTidyModule.cpp - clang-tidy ----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "../google/ExplicitConstructorCheck.h"
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../modernize/AvoidCArraysCheck.h"
 #include "../modernize/UseOverrideCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidGotoCheck.h"
 #include "AvoidNonConstGlobalVariablesCheck.h"
 #include "InitVariablesCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
         "cppcoreguidelines-avoid-c-arrays");
     CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
     CheckFactories.registerCheck<readability::MagicNumbersCheck>(
         "cppcoreguidelines-avoid-magic-numbers");
     CheckFactories.registerCheck<AvoidNonConstGlobalVariablesCheck>(
         "cppcoreguidelines-avoid-non-const-global-variables");
+    CheckFactories.registerCheck<google::ExplicitConstructorCheck>(
+        "cppcoreguidelines-explicit-constructor-and-conversion");
     CheckFactories.registerCheck<modernize::UseOverrideCheck>(
         "cppcoreguidelines-explicit-virtual-functions");
     CheckFactories.registerCheck<InitVariablesCheck>(
         "cppcoreguidelines-init-variables");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
     CheckFactories.registerCheck<MacroUsageCheck>(
         "cppcoreguidelines-macro-usage");

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

 ///
 /// See https://google.github.io/styleguide/cppguide.html#Explicit_Constructors
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/google-explicit-constructor.html
 class ExplicitConstructorCheck : public ClangTidyCheck {
 public:
   ExplicitConstructorCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+      : ClangTidyCheck(Name, Context),
+        ConstructorWhitelist(Options.get("ConstructorWhitelist", "")) {}
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const std::string ConstructorWhitelist;
 };
 
 } // namespace google
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_EXPLICITCONSTRUCTORCHECK_H

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

 //===--- ExplicitConstructorCheck.cpp - clang-tidy ------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ExplicitConstructorCheck.h"
+#include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace google {
+namespace {
+auto hasAnyWhitelistedName(const std::string &Names) {
+  const std::vector<std::string> NameList =
+      utils::options::parseStringList(Names);
+  return hasAnyName(std::vector<StringRef>(NameList.begin(), NameList.end()));
+}
+} // namespace
+
+void ExplicitConstructorCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "ConstructorWhitelist", ConstructorWhitelist);
+}
 
 void ExplicitConstructorCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
-      cxxConstructorDecl(unless(anyOf(isImplicit(), // Compiler-generated.
-                                      isDeleted(), isInstantiated())))
+      cxxConstructorDecl(
+          unless(anyOf(isImplicit(), // Compiler-generated.
+                       isDeleted(), isInstantiated(),
+                       hasAnyWhitelistedName(ConstructorWhitelist))))
           .bind("ctor"),
       this);
   Finder->addMatcher(
       cxxConversionDecl(unless(anyOf(isExplicit(), // Already marked explicit.
                                      isImplicit(), // Compiler-generated.
                                      isDeleted(), isInstantiated())))
 
           .bind("conversion"),
static bool isStdInitializerList(QualType Type) {
   return false;
 }
 
 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")) {
     if (Conversion->isOutOfLine())
       return;
     SourceLocation Loc = Conversion->getLocation();
     // Ignore all macros until we learn to ignore specific ones (e.g. used in
     // gmock to define matchers).
     if (Loc.isMacroID())
       return;
     diag(Loc, WarningMessage)

clang-tools-extra/docs/ReleaseNotes.rst

 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-pos47-c
   <clang-tidy/checks/cert-pos47-c>` to
   :doc:`concurrency-thread-canceltype-asynchronous
   <clang-tidy/checks/concurrency-thread-canceltype-asynchronous>` was added.
 
+- New alias :doc:`cppcoreguidelines-explicit-constructor-and-conversion
+  <clang-tidy/checks/cppcoreguidelines-explicit-constructor-and-conversion>` to
+  :doc:`google-explicit-constructor
+  <clang-tidy/checks/google-explicit-constructor>` was added.
+
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone-signal-handler>` check.
 
   Added an option to choose the set of allowed functions.
 
 - Improved :doc:`readability-uniqueptr-delete-release
   <clang-tidy/checks/readability-uniqueptr-delete-release>` check.
 
   Added an option to choose whether to refactor by calling the ``reset`` member
   function or assignment to ``nullptr``.
   Added support for pointers to ``std::unique_ptr``.
 
+- Improved :doc:`google-explicit-constructor
+  <clang-tidy/checks/google-explicit-constructor>` check.
+
+  Added an option to disable warnings for constructors contained in a
+  whitelist.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 - The readability-deleted-default check has been removed.
   
   The clang warning `Wdefaulted-function-deleted
   <https://clang.llvm.org/docs/DiagnosticsReference.html#wdefaulted-function-deleted>`_
   will diagnose the same issues and is enabled by default.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-explicit-constructor-and-conversion.rst

+.. title:: clang-tidy - cppcoreguidelines-explicit-constructor-and-conversion
+.. meta::
+   :http-equiv=refresh: 5;URL=google-explicit-constructor.html
+
+cppcoreguidelines-explicit-constructor
+======================================
+
+The cppcoreguidelines-explicit-constructor-and-conversion check is an alias,
+please see `google-explicit-constructor <google-explicit-constructor.html>`_
+for more information.
+
+This check implements `C.46 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c46-by-default-declare-single-argument-constructors-explicit>`_
+and `C.164 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c164-avoid-implicit-conversion-operators>`_
+from the CppCoreGuidelines.

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

struct S {
     explicit S(int a);
     S(const S&);
     explicit operator bool() const;
     ...
 
 
 
 See https://google.github.io/styleguide/cppguide.html#Explicit_Constructors
+
+Options
+-------
+
+.. option:: ConstructorWhitelist
+
+    Non-explicit single-argument constructors in this semicolon-separated list
+    will be ignored and will not trigger a warning. This option is used by this
+    check's `cppcoreguidelines-explicit-constructor-and-conversion <cppcoreguidelines-explicit-constructor-and-conversion.html>`_
+    alias to comply with the CppCoreGuidelines. The default list is empty.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table:: Aliases..
    `clang-analyzer-unix.MallocSizeof <clang-analyzer-unix.MallocSizeof.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,
    `clang-analyzer-unix.MismatchedDeallocator <clang-analyzer-unix.MismatchedDeallocator.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,
    `clang-analyzer-unix.Vfork <clang-analyzer-unix.Vfork.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,
    `clang-analyzer-unix.cstring.BadSizeArg <clang-analyzer-unix.cstring.BadSizeArg.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,
    `clang-analyzer-unix.cstring.NullArg <clang-analyzer-unix.cstring.NullArg.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,
    `cppcoreguidelines-avoid-c-arrays <cppcoreguidelines-avoid-c-arrays.html>`_, `modernize-avoid-c-arrays <modernize-avoid-c-arrays.html>`_,
    `cppcoreguidelines-avoid-magic-numbers <cppcoreguidelines-avoid-magic-numbers.html>`_, `readability-magic-numbers <readability-magic-numbers.html>`_,
    `cppcoreguidelines-c-copy-assignment-signature <cppcoreguidelines-c-copy-assignment-signature.html>`_, `misc-unconventional-assign-operator <misc-unconventional-assign-operator.html>`_,
+   `cppcoreguidelines-explicit-constructor-and-conversion <cppcoreguidelines-explicit-constructor-and-conversion.html>`_, `google-explicit-constructor <google-explicit-constructor.html>`_, "Yes"
    `cppcoreguidelines-explicit-virtual-functions <cppcoreguidelines-explicit-virtual-functions.html>`_, `modernize-use-override <modernize-use-override.html>`_, "Yes"
    `cppcoreguidelines-non-private-member-variables-in-classes <cppcoreguidelines-non-private-member-variables-in-classes.html>`_, `misc-non-private-member-variables-in-classes <misc-non-private-member-variables-in-classes.html>`_,
    `fuchsia-header-anon-namespaces <fuchsia-header-anon-namespaces.html>`_, `google-build-namespaces <google-build-namespaces.html>`_,
    `google-readability-braces-around-statements <google-readability-braces-around-statements.html>`_, `readability-braces-around-statements <readability-braces-around-statements.html>`_, "Yes"
    `google-readability-function-size <google-readability-function-size.html>`_, `readability-function-size <readability-function-size.html>`_,
    `google-readability-namespace-comments <google-readability-namespace-comments.html>`_, `llvm-namespace-comment <llvm-namespace-comment.html>`_,
    `hicpp-avoid-c-arrays <hicpp-avoid-c-arrays.html>`_, `modernize-avoid-c-arrays <modernize-avoid-c-arrays.html>`_,
    `hicpp-braces-around-statements <hicpp-braces-around-statements.html>`_, `readability-braces-around-statements <readability-braces-around-statements.html>`_, "Yes"

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

+// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \
+// RUN: google-explicit-constructor %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN: ]}'
+
+// RUN: %check_clang_tidy -check-suffix=WHITELISTED %s \
+// RUN: google-explicit-constructor %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN:   {key: google-explicit-constructor.ConstructorWhitelist, value: "A;B"} \
+// RUN: ]}'
+
+struct A {
+  A() {}
+  A(int x, int y) {}
+
+  explicit A(void *x) {}
+  explicit A(void *x, void *y) {}
+
+  explicit A(const A &a) {}
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:12: warning: copy constructor should not be declared explicit [google-explicit-constructor]
+  // CHECK-FIXES-DEFAULT: {{^  }}A(const A &a) {}
+  // WHITELISTED: Warning disabled with ConstructorWhitelist=A
+
+  A(int x1);
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
+  // CHECK-FIXES-DEFAULT: {{^  }}explicit A(int x1);
+  // WHITELISTED: Warning disabled with ConstructorWhitelist=A
+
+  A(double x2, double y = 3.14) {}
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: constructors that are callable with a single argument must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
+  // CHECK-FIXES-DEFAULT: {{^  }}explicit A(double x2, double y = 3.14) {}
+  // WHITELISTED: Warning disabled with ConstructorWhitelist=A
+
+  template <typename... T>
+  A(T &&...args);
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: constructors that are callable with a single argument
+  // CHECK-FIXES-DEFAULT: {{^  }}explicit A(T &&...args);
+  // WHITELISTED: Warning disabled with ConstructorWhitelist=A
+};
+
+inline A::A(int x1) {}
+
+struct B {
+  B() {}
+  B(int x, int y) {}
+
+  explicit B(void *x) {}
+  explicit B(void *x, void *y) {}
+
+  explicit B(const B &b) {}
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:12: warning: copy constructor should not be declared explicit [google-explicit-constructor]
+  // CHECK-FIXES-DEFAULT: {{^  }}B(const B &b) {}
+  // WHITELISTED: Warning disabled with ConstructorWhitelist=B
+
+  B(int x1);
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
+  // CHECK-FIXES-DEFAULT: {{^  }}explicit B(int x1);
+  // WHITELISTED: Warning disabled with ConstructorWhitelist=B
+
+  B(double x2, double y = 3.14) {}
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: constructors that are callable with a single argument must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
+  // CHECK-FIXES-DEFAULT: {{^  }}explicit B(double x2, double y = 3.14) {}
+  // WHITELISTED: Warning disabled with ConstructorWhitelist=B
+
+  template <typename... T>
+  B(T &&...args);
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: constructors that are callable with a single argument
+  // CHECK-FIXES-DEFAULT: {{^  }}explicit B(T &&...args);
+  // WHITELISTED: Warning disabled with ConstructorWhitelist=B
+};
+
+struct C {
+  C() {}
+  C(int x, int y) {}
+
+  explicit C(void *x) {}
+  explicit C(void *x, void *y) {}
+
+  explicit C(const C &c) {}
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:12: warning: copy constructor should not be declared explicit [google-explicit-constructor]
+  // CHECK-MESSAGES-WHITELISTED: :[[@LINE-2]]:12: warning: copy constructor should not be declared explicit [google-explicit-constructor]
+  // CHECK-FIXES: {{^  }}C(const C &c) {}
+
+  C(int x1);
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
+  // CHECK-MESSAGES-WHITELISTED: :[[@LINE-2]]:3: warning: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
+  // CHECK-FIXES: {{^  }}explicit C(int x1);
+
+  C(double x2, double y = 3.14) {}
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: constructors that are callable with a single argument must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
+  // CHECK-MESSAGES-WHITELISTED: :[[@LINE-2]]:3: warning: constructors that are callable with a single argument must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
+  // CHECK-FIXES: {{^  }}explicit C(double x2, double y = 3.14) {}
+
+  template <typename... T>
+  C(T &&...args);
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: constructors that are callable with a single argument
+  // CHECK-MESSAGES-WHITELISTED: :[[@LINE-2]]:3: warning: constructors that are callable with a single argument
+  // CHECK-FIXES: {{^  }}explicit C(T &&...args);
+};