[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/PiotrZSL closed 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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/HerrCai0907 approved this pull request. 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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/5chmidti approved this pull request. 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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/AMS21 updated https://github.com/llvm/llvm-project/pull/82689
>From 6bf8d649fb78b81adbaa7de82064161e14213fc4 Mon Sep 17 00:00:00 2001
From: AMS21
Date: Thu, 22 Feb 2024 22:10:09 +0100
Subject: [PATCH] [clang-tidy] Improve `google-explicit-constructor` checks
handling of `explicit(bool)`
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
---
.../google/ExplicitConstructorCheck.cpp | 33 ---
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++
.../google/explicit-constructor-cxx20.cpp | 59 +++
3 files changed, 88 insertions(+), 8 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp
diff --git a/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
b/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
index 34d49af9f81e23..6f26de9881357f 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 ) {
- 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 ) {
// 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 ) {
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 ) {
return Tok.is(tok::raw_identifier) &&
@@ -130,18 +134,31 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
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 3f90e7d63d6b23..f2df3d0d737c6b 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 00..95206f1ef420c3
--- /dev/null
+++
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/MacDue updated
https://github.com/llvm/llvm-project/pull/82689
>From 6bf8d649fb78b81adbaa7de82064161e14213fc4 Mon Sep 17 00:00:00 2001
From: AMS21
Date: Thu, 22 Feb 2024 22:10:09 +0100
Subject: [PATCH] [clang-tidy] Improve `google-explicit-constructor` checks
handling of `explicit(bool)`
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
---
.../google/ExplicitConstructorCheck.cpp | 33 ---
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++
.../google/explicit-constructor-cxx20.cpp | 59 +++
3 files changed, 88 insertions(+), 8 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp
diff --git a/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
b/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
index 34d49af9f81e23..6f26de9881357f 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 ) {
- 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 ) {
// 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 ) {
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 ) {
return Tok.is(tok::raw_identifier) &&
@@ -130,18 +134,31 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
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 3f90e7d63d6b23..f2df3d0d737c6b 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 00..95206f1ef420c3
--- /dev/null
+++
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
@@ -79,8 +79,10 @@ static bool isStdInitializerList(QualType Type) {
}
void ExplicitConstructorCheck::check(const MatchFinder::MatchResult ) {
- 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";
AMS21 wrote:
I agree sound better :)
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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/AMS21 updated https://github.com/llvm/llvm-project/pull/82689
>From ffde0d326b7ab2c451c9070a894d6c487fe90682 Mon Sep 17 00:00:00 2001
From: AMS21
Date: Thu, 22 Feb 2024 22:10:09 +0100
Subject: [PATCH] [clang-tidy] Improve `google-explicit-constructor` checks
handling of `explicit(bool)`
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
---
.../google/ExplicitConstructorCheck.cpp | 33 ---
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++
.../google/explicit-constructor-cxx20.cpp | 59 +++
3 files changed, 88 insertions(+), 8 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp
diff --git a/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
b/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
index 34d49af9f81e23..6f26de9881357f 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 ) {
- 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 ) {
// 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 ) {
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 ) {
return Tok.is(tok::raw_identifier) &&
@@ -130,18 +134,31 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
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 3f90e7d63d6b23..f2df3d0d737c6b 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 00..e47538babbbc16
--- /dev/null
+++
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
@@ -79,8 +79,10 @@ static bool isStdInitializerList(QualType Type) {
}
void ExplicitConstructorCheck::check(const MatchFinder::MatchResult ) {
- 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";
PiotrZSL wrote:
Maybe ? ```evaluates to `false` ```
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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/PiotrZSL approved this pull request. LGTM 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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/PiotrZSL edited 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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/AMS21 updated https://github.com/llvm/llvm-project/pull/82689
>From 73e7c7d4d6d1752d0da6bfef9d933da418df71a5 Mon Sep 17 00:00:00 2001
From: AMS21
Date: Thu, 22 Feb 2024 22:10:09 +0100
Subject: [PATCH] [clang-tidy] Improve `google-explicit-constructor` checks
handling of `explicit(bool)`
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
---
.../google/ExplicitConstructorCheck.cpp | 33 ---
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++
.../google/explicit-constructor-cxx20.cpp | 59 +++
3 files changed, 88 insertions(+), 8 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp
diff --git a/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
b/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
index 34d49af9f81e23..0e33c15f0e4949 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 ) {
- 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("conversion")) {
@@ -91,7 +93,7 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
// 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 ) {
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 ) {
return Tok.is(tok::raw_identifier) &&
@@ -130,18 +134,31 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
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 69537964f9bce0..02b3f27dd9eec2 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -147,6 +147,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 00..e47538babbbc16
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
@@ -147,6 +147,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)`. EugeneZelenko wrote: `C++20`. Please use double back-ticks for language constructs. 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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
AMS21 wrote: Rebased and addressed review comments 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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/AMS21 updated https://github.com/llvm/llvm-project/pull/82689
>From 93e17e4a53ddbc8203951617d54b146be7ffc0fb Mon Sep 17 00:00:00 2001
From: AMS21
Date: Thu, 22 Feb 2024 22:10:09 +0100
Subject: [PATCH] [clang-tidy] Improve `google-explicit-constructor` checks
handling of `explicit(bool)`
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
---
.../google/ExplicitConstructorCheck.cpp | 33 ---
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++
.../google/explicit-constructor-cxx20.cpp | 59 +++
3 files changed, 88 insertions(+), 8 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp
diff --git a/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
b/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
index 34d49af9f81e23..0e33c15f0e4949 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 ) {
- 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("conversion")) {
@@ -91,7 +93,7 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
// 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 ) {
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 ) {
return Tok.is(tok::raw_identifier) &&
@@ -130,18 +134,31 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
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 69537964f9bce0..9596a09cf2c338 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -147,6 +147,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 00..e47538babbbc16
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp
@@
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/5chmidti edited 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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/5chmidti edited 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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
@@ -130,18 +134,30 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
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(ExplicitExpr))
+ return;
+ }
+
5chmidti wrote:
I think that dependent expressions (`isInstantiationDependent`) should also be
ignored, e.g.
```c++
template
struct I {
explicit(Val > 0) I(int);
};
template
struct J {
explicit(Val > 0) J(int);
};
void useJ(J<0>, J<100>);
```
should not produce any warnings IMO.
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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
@@ -130,18 +134,30 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
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(ExplicitExpr))
+ return;
+ }
+
bool SingleArgument =
EugeneZelenko wrote:
Should be `const`.
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
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
llvmbot wrote:
@llvm/pr-subscribers-clang-tools-extra
Author: None (AMS21)
Changes
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 ) {
- 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("conversion")) {
@@ -91,7 +93,7 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
// 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 ) {
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 ) {
return Tok.is(tok::raw_identifier) &&
@@ -130,18 +134,30 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
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(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
` 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 00..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 {
+
[clang-tools-extra] [clang-tidy] Improve `google-explicit-constructor` checks handling of `explicit(bool)` (PR #82689)
https://github.com/AMS21 created https://github.com/llvm/llvm-project/pull/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
>From 81609e762417c0ea19a869cf35ced75522e2be6b Mon Sep 17 00:00:00 2001
From: AMS21
Date: Thu, 22 Feb 2024 22:10:09 +0100
Subject: [PATCH] [clang-tidy] Improve `google-explicit-constructor` checks
handling of `explicit(bool)`
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
---
.../google/ExplicitConstructorCheck.cpp | 30 +---
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++
.../google/explicit-constructor-cxx20.cpp | 47 +++
3 files changed, 74 insertions(+), 7 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/google/explicit-constructor-cxx20.cpp
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 ) {
- 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("conversion")) {
@@ -91,7 +93,7 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
// 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 ) {
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 ) {
return Tok.is(tok::raw_identifier) &&
@@ -130,18 +134,30 @@ void ExplicitConstructorCheck::check(const
MatchFinder::MatchResult ) {
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(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
` 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
