-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang] Add diagnostic for usage of implicit constructor with pointer to bool convertion #143990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… to bool convertion
Like
```c++
struct B {
B(bool V) {}
};
void test(const B& b);
void test0(B* b) {
test(b); // HERE
}
```
|
@llvm/pr-subscribers-clang Author: None (Ralender) ChangesThis emits warnings for this code instead of silently creating a new B in place. struct B {
B(bool V) {}
};
void test(const B& b);
void test0(B* b) {
test(b); // HERE
}Full diff: https://github.com/llvm/llvm-project/pull/143990.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 0f77083dac9df..8a17b04ad91ed 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4397,6 +4397,9 @@ def ext_ms_impcast_fn_obj : ExtWarn<
"implicit conversion between pointer-to-function and pointer-to-object is a "
"Microsoft extension">, InGroup<MicrosoftCast>;
+def warn_imp_constructor_pointer_to_bool : Warning<
+ "implicit conversion from %0 to %1 calls %q2; maybe you intended to dereference">,
+ InGroup<PointerBoolConversion>;
def warn_impcast_pointer_to_bool : Warning<
"address of %select{'%1'|function '%1'|array '%1'|lambda function pointer "
"conversion operator}0 will always evaluate to 'true'">,
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 8f8e1ceb7197e..d0d52f3a1fe87 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11767,6 +11767,25 @@ static void CheckImplicitArgumentConversions(Sema &S, const CallExpr *TheCall,
SourceLocation CC) {
for (unsigned I = 0, N = TheCall->getNumArgs(); I < N; ++I) {
const Expr *CurrA = TheCall->getArg(I);
+
+ if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(CurrA))
+ // We shouldnt skip over any node here as it may be an attempt to silence
+ // the warning
+ if (auto *CCE = dyn_cast<CXXConstructExpr>(MTE->getSubExpr()))
+ if (CCE->getNumArgs() == 1) {
+ Expr *Inner = CCE->getArg(0)->IgnoreImpCasts();
+ if ((Inner->getType()->isAnyPointerType() &&
+ Inner->getType()->getPointeeType().getUnqualifiedType() ==
+ CCE->getType().getUnqualifiedType())) {
+ S.Diag(CCE->getLocation(),
+ diag::warn_imp_constructor_pointer_to_bool)
+ << Inner->getType() << CCE->getType() << CCE->getConstructor();
+ S.Diag(CCE->getConstructor()->getLocation(),
+ diag::note_entity_declared_at)
+ << CCE->getConstructor();
+ }
+ }
+
if (!IsImplicitBoolFloatConversion(S, CurrA, true))
continue;
diff --git a/clang/test/SemaCXX/warn-bool-conversion.cpp b/clang/test/SemaCXX/warn-bool-conversion.cpp
index 18c35776b17bc..52d5315e7cb3b 100644
--- a/clang/test/SemaCXX/warn-bool-conversion.cpp
+++ b/clang/test/SemaCXX/warn-bool-conversion.cpp
@@ -234,3 +234,34 @@ namespace Template {
template void h<d>();
}
#endif // __cplusplus < 201703L
+
+namespace implicit_constructor_bool {
+
+struct B {
+ bool a;
+ B(bool V) : a(V) {} // expected-note {{'B' declared here}}
+};
+
+void test(const B& b);
+
+void test0(B* b) {
+ test(b); // expected-warning {{implicit conversion from 'B *' to 'const B' calls}}
+ test((const B&)b);
+ test(B(b));
+ test((bool)b);
+ test(static_cast<bool>(b));
+ test(*b);
+}
+
+struct C {
+ bool a;
+ explicit C(bool V) : a(V) {}
+};
+
+void testC(const C& b); // expected-note {{candidate function not viable: no known conversion from 'C *' to 'const C'}}
+
+void testC0(C* b) {
+ testC(b); // expected-error {{no matching function for call to 'testC'}}
+}
+
+}
|
AaronBallman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this (and sorry for the delayed review)! Please be sure to add a release note to clang/docs/ReleaseNotes.rst so users know about the improved diagnostics.
| "Microsoft extension">, InGroup<MicrosoftCast>; | ||
|
|
||
| def warn_imp_constructor_pointer_to_bool : Warning< | ||
| "implicit conversion from %0 to %1 calls %q2; maybe you intended to dereference">, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the 'maybe' part at least requires some work. Typically we'd do something like did you intend to dereference %0? Or did you mean *%0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tell me if the new syntax + fixit is good.
| "Microsoft extension">, InGroup<MicrosoftCast>; | ||
|
|
||
| def warn_imp_constructor_pointer_to_bool : Warning< | ||
| "implicit conversion from %0 to %1 calls %q2; did you intend to dereference ?">, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "implicit conversion from %0 to %1 calls %q2; did you intend to dereference ?">, | |
| "implicit conversion from %0 to %1 calls %q2; did you intend to dereference instead?">, |
@erichkeane do you think we should have fixits attached to notes? Something like:
implicit conversion from %0 to %1 calls %q2 -- warning on the code
did you intend to dereference? -- with fixit to insert *
did you intend the bool conversion? -- with fixit to insert a (static_)cast or the !! trick
My concern is: if the user intended this behavior, it'd be nice for them to have a way to silence the diagnostic. And if they didn't intend this behavior, giving them a fix-it helps them correct their code.
This emits warnings for this code instead of silently creating a new B in place.