llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: None (AMS21) <details> <summary>Changes</summary> We now treat `explicit(false)` the same way we treat `noexcept(false)` in the noexcept checks, which is ignoring it. Also introduced a new warning message if a constructor has an `explicit` declaration which evaluates to false and no longer emit a faulty FixIt. Fixes #<!-- -->81121 --- Full diff: https://github.com/llvm/llvm-project/pull/82689.diff 3 Files Affected: - (modified) clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp (+23-7) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) - (added) clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp (+47) ``````````diff diff --git a/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp b/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp index 34d49af9f81e23..3b91b800a9218f 100644 --- a/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp +++ b/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp @@ -79,8 +79,10 @@ static bool isStdInitializerList(QualType Type) { } void ExplicitConstructorCheck::check(const MatchFinder::MatchResult &Result) { - constexpr char WarningMessage[] = + constexpr char NoExpressionWarningMessage[] = "%0 must be marked explicit to avoid unintentional implicit conversions"; + constexpr char WithExpressionWarningMessage[] = + "%0 explicit expression evaluated to false"; if (const auto *Conversion = Result.Nodes.getNodeAs<CXXConversionDecl>("conversion")) { @@ -91,7 +93,7 @@ void ExplicitConstructorCheck::check(const MatchFinder::MatchResult &Result) { // gmock to define matchers). if (Loc.isMacroID()) return; - diag(Loc, WarningMessage) + diag(Loc, NoExpressionWarningMessage) << Conversion << FixItHint::CreateInsertion(Loc, "explicit "); return; } @@ -101,9 +103,11 @@ void ExplicitConstructorCheck::check(const MatchFinder::MatchResult &Result) { Ctor->getMinRequiredArguments() > 1) return; + const ExplicitSpecifier ExplicitSpec = Ctor->getExplicitSpecifier(); + bool TakesInitializerList = isStdInitializerList( Ctor->getParamDecl(0)->getType().getNonReferenceType()); - if (Ctor->isExplicit() && + if (ExplicitSpec.isExplicit() && (Ctor->isCopyOrMoveConstructor() || TakesInitializerList)) { auto IsKwExplicit = [](const Token &Tok) { return Tok.is(tok::raw_identifier) && @@ -130,18 +134,30 @@ void ExplicitConstructorCheck::check(const MatchFinder::MatchResult &Result) { return; } - if (Ctor->isExplicit() || Ctor->isCopyOrMoveConstructor() || + if (ExplicitSpec.isExplicit() || Ctor->isCopyOrMoveConstructor() || TakesInitializerList) return; + // Don't complain about explicit(false) + const Expr *ExplicitExpr = ExplicitSpec.getExpr(); + if (ExplicitExpr) { + ExplicitExpr = ExplicitExpr->IgnoreImplicit(); + if (isa<CXXBoolLiteralExpr>(ExplicitExpr)) + return; + } + bool SingleArgument = Ctor->getNumParams() == 1 && !Ctor->getParamDecl(0)->isParameterPack(); SourceLocation Loc = Ctor->getLocation(); - diag(Loc, WarningMessage) + auto Diag = + diag(Loc, ExplicitExpr ? WithExpressionWarningMessage + : NoExpressionWarningMessage) << (SingleArgument ? "single-argument constructors" - : "constructors that are callable with a single argument") - << FixItHint::CreateInsertion(Loc, "explicit "); + : "constructors that are callable with a single argument"); + + if (!ExplicitExpr) + Diag << FixItHint::CreateInsertion(Loc, "explicit "); } } // namespace clang::tidy::google diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index a0b9fcfe0d7774..f57a963a8e66a1 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -143,6 +143,10 @@ Changes in existing checks <clang-tidy/checks/google/build-namespaces>` check by replacing the local option `HeaderFileExtensions` by the global option of the same name. +- Improved :doc:`google-explicit-constructor + <clang-tidy/checks/google/explicit-constructor>` check to better handle + C++-20 `explicit(bool)`. + - Improved :doc:`google-global-names-in-headers <clang-tidy/checks/google/global-names-in-headers>` check by replacing the local option `HeaderFileExtensions` by the global option of the same name. diff --git a/clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp b/clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp new file mode 100644 index 00000000000000..4e2fcbecf3e948 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp @@ -0,0 +1,47 @@ +// RUN: %check_clang_tidy %s google-explicit-constructor %t -std=c++20-or-later + +namespace issue_81121 +{ + +static constexpr bool ConstFalse = false; +static constexpr bool ConstTrue = true; + +struct A { + explicit(true) A(int); +}; + +struct B { + explicit(false) B(int); +}; + +struct C { + explicit(ConstTrue) C(int); +}; + +struct D { + explicit(ConstFalse) D(int); + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: single-argument constructors explicit expression evaluated to false [google-explicit-constructor] +}; + +template <typename> +struct E { + explicit(true) E(int); +}; + +template <typename> +struct F { + explicit(false) F(int); +}; + +template <typename> +struct G { + explicit(ConstTrue) G(int); +}; + +template <typename> +struct H { + explicit(ConstFalse) H(int); + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: single-argument constructors explicit expression evaluated to false [google-explicit-constructor] +}; + +} // namespace issue_81121 `````````` </details> https://github.com/llvm/llvm-project/pull/82689 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
