diff --git a/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp b/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp index 34d49af9f81e2..6f26de9881357 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 evaluates to 'false'"; if (const auto *Conversion = Result.Nodes.getNodeAs("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,31 @@ void ExplicitConstructorCheck::check(const MatchFinder::MatchResult &Result) { return; } - if (Ctor->isExplicit() || Ctor->isCopyOrMoveConstructor() || + if (ExplicitSpec.isExplicit() || Ctor->isCopyOrMoveConstructor() || TakesInitializerList) return; - bool SingleArgument = + // Don't complain about explicit(false) or dependent expressions + const Expr *ExplicitExpr = ExplicitSpec.getExpr(); + if (ExplicitExpr) { + ExplicitExpr = ExplicitExpr->IgnoreImplicit(); + if (isa(ExplicitExpr) || + ExplicitExpr->isInstantiationDependent()) + return; + } + + const 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 3f90e7d63d6b2..f2df3d0d737c6 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -151,6 +151,10 @@ Changes in existing checks ` check by replacing the local option `HeaderFileExtensions` by the global option of the same name. +- Improved :doc:`google-explicit-constructor + ` check to better handle + ``C++-20`` `explicit(bool)`. + - Improved :doc:`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 0000000000000..95206f1ef420c --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp @@ -0,0 +1,59 @@ +// 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 evaluates to 'false' [google-explicit-constructor] +}; + +template +struct E { + explicit(true) E(int); +}; + +template +struct F { + explicit(false) F(int); +}; + +template +struct G { + explicit(ConstTrue) G(int); +}; + +template +struct H { + explicit(ConstFalse) H(int); + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: single-argument constructors explicit expression evaluates to 'false' [google-explicit-constructor] +}; + +template +struct I { + explicit(Val > 0) I(int); +}; + +template +struct J { + explicit(Val > 0) J(int); +}; + +void useJ(J<0>, J<100>); + +} // namespace issue_81121