diff options
author | AMS21 <AMS21.github@gmail.com> | 2024-03-03 12:13:25 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-03-03 12:13:25 +0100 |
commit | eb3b063995d6b4f8f3bc22eeecbf239ffaecc29f (patch) | |
tree | da1212d18761a6c50a504e37c8e304668103a05b /clang-tools-extra | |
parent | 800de14fab136f8e17c04cc783e0f8f2b305333b (diff) | |
download | llvm-eb3b063995d6b4f8f3bc22eeecbf239ffaecc29f.zip llvm-eb3b063995d6b4f8f3bc22eeecbf239ffaecc29f.tar.gz llvm-eb3b063995d6b4f8f3bc22eeecbf239ffaecc29f.tar.bz2 |
[clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (#82689)
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
Diffstat (limited to 'clang-tools-extra')
3 files changed, 88 insertions, 8 deletions
diff --git a/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp b/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp index 34d49af..6f26de9 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<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,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<CXXBoolLiteralExpr>(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 5bae530..0d24672 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -157,6 +157,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 0000000..95206f1 --- /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 <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 evaluates to 'false' [google-explicit-constructor] +}; + +template <int Val> +struct I { + explicit(Val > 0) I(int); +}; + +template <int Val> +struct J { + explicit(Val > 0) J(int); +}; + +void useJ(J<0>, J<100>); + +} // namespace issue_81121 |