[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 ↗

(On Diff #346640)

23–24 ↗

(On Diff #346640)

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 ↗

(On Diff #346640)

Unintended formatting change?

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

28 ↗

(On Diff #346640)

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 ↗

(On Diff #346640)

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 ↗

(On Diff #346640)

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

102 ↗

(On Diff #346640)

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.

28–30 ↗

(On Diff #347970)

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 ↗

(On Diff #347970)

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

74–75 ↗

(On Diff #347970)

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 ↗

(On Diff #348178)

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 ↗

(On Diff #347970)

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

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/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"