Skip to content

Commit

Permalink
[clang-tidy] Improve google-explicit-constructor checks handling of…
Browse files Browse the repository at this point in the history
… `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
  • Loading branch information
AMS21 committed Mar 3, 2024
1 parent 800de14 commit eb3b063
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 8 deletions.
33 changes: 25 additions & 8 deletions clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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")) {
Expand All @@ -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;
}
Expand All @@ -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) &&
Expand All @@ -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
4 changes: 4 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit eb3b063

Please sign in to comment.