[clang] Fix -Wdouble-promotion in C++ list-initialization #159992
Conversation
A C++ list-initialization explicitly asks for the promotion to happen, much like an explicit static_cast, so it should not be warned about. Fixes llvm#33409
Add tests to ensure that -Wdouble-promotion does not warn when promotion is asked for explicitly by an explicit cast or C++ list initialization. For the latter this creates a .cpp version of warn-double-promotion.c. Test case for llvm#33409
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Marcel Jacobse (mjacobse) ChangesResolves #33409. The information Also adds tests, both for the changed list-initialization case as well as for normal explicit casts which already would have passed before this PR. These negative tests are added directly next to the positive tests in Full diff: https://github.com/llvm/llvm-project/pull/159992.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 46d56bb3f07f5..37d5e09c3f4e8 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -417,6 +417,7 @@ Bug Fixes to C++ Support
``__builtin_addressof``, and related issues with builtin arguments. (#GH154034)
- Fix an assertion failure when taking the address on a non-type template parameter argument of
object type. (#GH151531)
+- Suppress ``-Wdouble-promotion`` when explicitly asked for with C++ list initialization (#GH33409).
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 00f40cfa910d2..d5b2cde0d1c09 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -12382,6 +12382,11 @@ void Sema::CheckImplicitConversion(Expr *E, QualType T, SourceLocation CC,
}
// ... or possibly if we're increasing rank, too
else if (Order < 0) {
+ // Don't warn if we are in a C++ list initialization expression, as
+ // that means the promotion was asked for explicitly.
+ if (IsListInit)
+ return;
+
if (SourceMgr.isInSystemMacro(CC))
return;
diff --git a/clang/test/Sema/warn-double-promotion.c b/clang/test/Sema/warn-double-promotion.c
index 5742a4fb3cbd4..ac9e9499bc2b7 100644
--- a/clang/test/Sema/warn-double-promotion.c
+++ b/clang/test/Sema/warn-double-promotion.c
@@ -24,10 +24,25 @@ long double ReturnLongDoubleFromDouble(double d) {
return d; //expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
}
+double ReturnDoubleFromFloatWithExplicitCast(float f) {
+ return (double)f;
+}
+
+long double ReturnLongDoubleFromFloatWithExplicitCast(float f) {
+ return (long double)f;
+}
+
+long double ReturnLongDoubleFromDoubleWithExplicitCast(double d) {
+ return (long double)d;
+}
+
void Assignment(float f, double d, long double ld) {
d = f; //expected-warning{{implicit conversion increases floating-point precision: 'float' to 'double'}}
ld = f; //expected-warning{{implicit conversion increases floating-point precision: 'float' to 'long double'}}
ld = d; //expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+ d = (double)f;
+ ld = (long double)f;
+ ld = (long double)d;
f = d;
f = ld;
d = ld;
@@ -40,6 +55,9 @@ void ArgumentPassing(float f, double d) {
DoubleParameter(f); // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'double'}}
LongDoubleParameter(f); // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'long double'}}
LongDoubleParameter(d); // expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+ DoubleParameter((double)f);
+ LongDoubleParameter((long double)f);
+ LongDoubleParameter((long double)d);
}
void BinaryOperator(float f, double d, long double ld) {
@@ -49,12 +67,21 @@ void BinaryOperator(float f, double d, long double ld) {
f = ld * f; // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'long double'}}
d = d * ld; // expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
d = ld * d; // expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+ f = (double)f * d;
+ f = d * (double)f;
+ f = (long double)f * ld;
+ f = ld * (long double)f;
+ d = (long double)d * ld;
+ d = ld * (long double)d;
}
void MultiplicationAssignment(float f, double d, long double ld) {
d *= f; // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'double'}}
ld *= f; // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'long double'}}
ld *= d; // expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+ d *= (double)f;
+ ld *= (long double)f;
+ ld *= (long double)d;
// FIXME: These cases should produce warnings as above.
f *= d;
diff --git a/clang/test/Sema/warn-double-promotion.cpp b/clang/test/Sema/warn-double-promotion.cpp
new file mode 100644
index 0000000000000..677f59a219521
--- /dev/null
+++ b/clang/test/Sema/warn-double-promotion.cpp
@@ -0,0 +1,128 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -verify -fsyntax-only %s -Wdouble-promotion
+
+float ReturnFloatFromDouble(double d) {
+ return d;
+}
+
+float ReturnFloatFromLongDouble(long double ld) {
+ return ld;
+}
+
+double ReturnDoubleFromLongDouble(long double ld) {
+ return ld;
+}
+
+double ReturnDoubleFromFloat(float f) {
+ return f; //expected-warning{{implicit conversion increases floating-point precision: 'float' to 'double'}}
+}
+
+long double ReturnLongDoubleFromFloat(float f) {
+ return f; //expected-warning{{implicit conversion increases floating-point precision: 'float' to 'long double'}}
+}
+
+long double ReturnLongDoubleFromDouble(double d) {
+ return d; //expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+}
+
+double ReturnDoubleFromFloatWithExplicitCast(float f) {
+ return static_cast<double>(f);
+}
+
+long double ReturnLongDoubleFromFloatWithExplicitCast(float f) {
+ return static_cast<long double>(f);
+}
+
+long double ReturnLongDoubleFromDoubleWithExplicitCast(double d) {
+ return static_cast<long double>(d);
+}
+
+double ReturnDoubleFromFloatWithExplicitListInitialization(float f) {
+ return double{f};
+}
+
+long double ReturnLongDoubleFromFloatWithExplicitListInitialization(float f) {
+ return (long double){f};
+}
+
+long double ReturnLongDoubleFromDoubleWithExplicitListInitialization(double d) {
+ return (long double){d};
+}
+
+void Assignment(float f, double d, long double ld) {
+ d = f; //expected-warning{{implicit conversion increases floating-point precision: 'float' to 'double'}}
+ ld = f; //expected-warning{{implicit conversion increases floating-point precision: 'float' to 'long double'}}
+ ld = d; //expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+ d = static_cast<double>(f);
+ ld = static_cast<long double>(f);
+ ld = static_cast<long double>(d);
+ d = double{f};
+ ld = (long double){f};
+ ld = (long double){d};
+ f = d;
+ f = ld;
+ d = ld;
+}
+
+extern void DoubleParameter(double);
+extern void LongDoubleParameter(long double);
+
+void ArgumentPassing(float f, double d) {
+ DoubleParameter(f); // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'double'}}
+ LongDoubleParameter(f); // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'long double'}}
+ LongDoubleParameter(d); // expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+ DoubleParameter(static_cast<double>(f));
+ LongDoubleParameter(static_cast<long double>(f));
+ LongDoubleParameter(static_cast<long double>(d));
+ DoubleParameter(double{f});
+ LongDoubleParameter((long double){f});
+ LongDoubleParameter((long double){d});
+}
+
+void BinaryOperator(float f, double d, long double ld) {
+ f = f * d; // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'double'}}
+ f = d * f; // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'double'}}
+ f = f * ld; // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'long double'}}
+ f = ld * f; // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'long double'}}
+ d = d * ld; // expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+ d = ld * d; // expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+ f = static_cast<double>(f) * d;
+ f = d * static_cast<double>(f);
+ f = static_cast<long double>(f) * ld;
+ f = ld * static_cast<long double>(f);
+ d = static_cast<long double>(d) * ld;
+ d = ld * static_cast<long double>(d);
+ f = double{f} * d;
+ f = d * double{f};
+ f = (long double){f} * ld;
+ f = ld * (long double){f};
+ d = (long double){d} * ld;
+ d = ld * (long double){d};
+}
+
+void MultiplicationAssignment(float f, double d, long double ld) {
+ d *= f; // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'double'}}
+ ld *= f; // expected-warning{{implicit conversion increases floating-point precision: 'float' to 'long double'}}
+ ld *= d; // expected-warning{{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+ d *= static_cast<double>(f);
+ ld *= static_cast<long double>(f);
+ ld *= static_cast<long double>(d);
+ d *= double{f};
+ ld *= (long double){f};
+ ld *= (long double){d};
+
+ // FIXME: These cases should produce warnings as above.
+ f *= d;
+ f *= ld;
+ d *= ld;
+}
+
+// FIXME: As with a binary operator, the operands to the conditional operator are
+// converted to a common type and should produce a warning.
+void ConditionalOperator(float f, double d, long double ld, int i) {
+ f = i ? f : d;
+ f = i ? d : f;
+ f = i ? f : ld;
+ f = i ? ld : f;
+ d = i ? d : ld;
+ d = i ? ld : d;
+}
|
|
|
Sirraide
left a comment
Sirraide
left a comment
There was a problem hiding this comment.
The implementation lgtm, but I think we can do some deduplication in the tests by adding a second RUN line to the .c file that specifies -x c++ and then only adding tests for truly C++-specific constructs in the .cpp file.
Also, while you’re at it, could you add some tests for functional-style casts (e.g. double(f)) as well as some tests involving templates (e.g. T{f} where T is a template parameter instantiated as double).
|
Also, it’d probably also be a good idea to add tests such as float f;
struct A {
A(double f) {}
};
A a{f};
struct B {
double d;
B(float f) : d{f} {}
};
B a{f};to make sure we only suppress the diagnostic if user actually wrote |
|
Thanks, your B(float f) : d(f) {}would bring back the warning. Something similar happens with local variable initialization too: void test(float f) {
double d0 = f;
double d1(f);
double d2{f};
}Before this PR, all three initializations give the warning. After this PR, I could see arguments for |
Yeah, imo those should all warn and if you don’t want a warning you have to write e.g. |
With (long double){f} we were getting the compound literal construction
from C99 instead of the intended C++ function-style cast with
list-initialization syntax.
Run warn-double-promotion.c in both C and C++ mode and only add those C++ tests with warn-double-promotion.cpp that are not valid C.
This reverts commit 4f63f70 which was the previous attempt to suppress -Wdouble-promotion when explicitly asked for with C++ list initialization. Just checking if we are somehow in a list initialization was way too loose and suppressed many cases that should not have been. The new fix works with the observation that the case of explicit C++ list initialization in this case turns into a CXXFunctionalCastExpr with InitListExpr as direct child. While the CXXFunctionalCastExpr is ignored when checking for implicit conversions, the InitListExpr is not, which causes the -Wdouble-promotion warning. To fix this, treat the InitListExpr as part of the CXXFunctionalCastExpr and skip it too.
|
Yeah that makes sense to me. So I reverted this attempt of just checking if we are in some kind of ListInit, it does not work. It looks like expressions like The implementation is not the prettiest though and it seems to rely heavily on this implicit assumption that I also applied the test deduplication that you suggested and added some tests for classic function-style casts and the initialization examples that showed that the previous fix was broken. I can add further suggested tests involving classes and templates later if the new fix turns out to be the right idea. I also introduced an alias for |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Sirraide
left a comment
Sirraide
left a comment
There was a problem hiding this comment.
I’ve thought about this for a bit, and this approach seems fine to me
While that is the case, I’d argue that hopefully no-one is using a compound literal for I don’t think we actually care to fix that though, so I think it’s fine to leave that as-is (maybe add a test case for it maybe to make sure we don’t crash on it). |
To make it more clear what's going on
Includes tests with classes and templates
Co-authored-by: Sirraide <[email protected]>
mjacobse
deleted the
fix_explicit_brace_init_double_promotion_warning
branch
|
@mjacobse Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
…m#159992) Resolves llvm#33409. The information `IsListInit` is already passed to function `CheckImplicitConversion` for another use-case which makes adding a condition for the double-promotion case simple. Also adds tests, both for the changed list-initialization case as well as for normal explicit casts which already would have passed before this PR. These negative tests are added directly next to the positive tests in `warn-double-promotion.c` or for the C++-specific cases in a new .cpp version of that file.
3 participants
Resolves #33409.
The information
IsListInitis already passed to functionCheckImplicitConversionfor another use-case which makes adding a condition for the double-promotion case simple.Also adds tests, both for the changed list-initialization case as well as for normal explicit casts which already would have passed before this PR. These negative tests are added directly next to the positive tests in
warn-double-promotion.cor for the C++-specific cases in a new .cpp version of that file.