Skip to content
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

[clang] Implement CWG2851: floating-point conversions in converted constant expressions #90387

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MitalAshok
Copy link
Contributor

https://cplusplus.github.io/CWG/issues/2851.html

The only time the target type for a converted constant expression is a floating-point type is in a NTTP, so this only affects C++20+.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 28, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 28, 2024

@llvm/pr-subscribers-clang

Author: Mital Ashok (MitalAshok)

Changes

https://cplusplus.github.io/CWG/issues/2851.html

The only time the target type for a converted constant expression is a floating-point type is in a NTTP, so this only affects C++20+.


Full diff: https://github.com/llvm/llvm-project/pull/90387.diff

4 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+4)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+35-3)
  • (modified) clang/test/CXX/drs/dr28xx.cpp (+35)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a1390d6536b28c..7c8d83bd73613b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -164,6 +164,9 @@ Resolutions to C++ Defect Reports
 - Clang now diagnoses declarative nested-name-specifiers with pack-index-specifiers.
   (`CWG2858: Declarative nested-name-specifiers and pack-index-specifiers <https://cplusplus.github.io/CWG/issues/2858.html>`_).
 
+- Allow floating-point promotions and conversions in converted constant expressions.
+  (`CWG2851 Allow floating-point conversions in converted constant expressions <https://cplusplus.github.io/CWG/issues/2851.html>`_).
+
 C Language Changes
 ------------------
 
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index fdca82934cb4dc..08ebb28ff17b25 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -85,6 +85,10 @@ def err_expr_not_cce : Error<
   "%select{case value|enumerator value|non-type template argument|"
   "array size|explicit specifier argument|noexcept specifier argument|"
   "call to 'size()'|call to 'data()'}0 is not a constant expression">;
+def err_float_conv_cant_represent : Error<
+  "non-type template argument evaluates to %0 which can not be "
+  "represented in type %1"
+>;
 def ext_cce_narrowing : ExtWarn<
   "%select{case value|enumerator value|non-type template argument|"
   "array size|explicit specifier argument|noexcept specifier argument|"
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 04cd9e78739d20..c35517db946b1c 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -6072,6 +6072,10 @@ static bool CheckConvertedConstantConversions(Sema &S,
   case ICK_Integral_Promotion:
   case ICK_Integral_Conversion: // Narrowing conversions are checked elsewhere.
   case ICK_Zero_Queue_Conversion:
+  // Per CWG2851, floating-point promotions and conversions are allowed.
+  // The value of a conversion is checked afterwards.
+  case ICK_Floating_Promotion:
+  case ICK_Floating_Conversion:
     return true;
 
   case ICK_Boolean_Conversion:
@@ -6091,9 +6095,7 @@ static bool CheckConvertedConstantConversions(Sema &S,
     // only permitted if the source type is std::nullptr_t.
     return SCS.getFromType()->isNullPtrType();
 
-  case ICK_Floating_Promotion:
   case ICK_Complex_Promotion:
-  case ICK_Floating_Conversion:
   case ICK_Complex_Conversion:
   case ICK_Floating_Integral:
   case ICK_Compatible_Conversion:
@@ -6229,7 +6231,37 @@ static ExprResult BuildConvertedConstantExpression(Sema &S, Expr *From,
   if (Result.isInvalid())
     return Result;
 
-  // Check for a narrowing implicit conversion.
+  if (SCS->Second == ICK_Floating_Conversion) {
+    // Unlike with narrowing conversions, the value must fit
+    // exactly even if it is in range
+    assert(CCE == Sema::CCEKind::CCEK_TemplateArg &&
+           "Only non-type template args should use floating-point conversions");
+
+    // Initializer is From, except it is a full-expression
+    const Expr *Initializer =
+        IgnoreNarrowingConversion(S.Context, Result.get());
+
+    // If it's value-dependent, we can't tell whether it will fit
+    if (Initializer->isValueDependent())
+      return Result;
+
+    if (!Initializer->isCXX11ConstantExpr(S.Context, &PreNarrowingValue)) {
+      S.Diag(Initializer->getBeginLoc(), diag::err_expr_not_cce) << CCE;
+      return Result;
+    }
+
+    llvm::APFloat PostNarrowingValue = PreNarrowingValue.getFloat();
+    bool LosesInfo = true;
+    PostNarrowingValue.convert(S.Context.getFloatTypeSemantics(T),
+                               llvm::APFloat::rmNearestTiesToEven, &LosesInfo);
+    if (LosesInfo)
+      S.Diag(From->getBeginLoc(), diag::err_float_conv_cant_represent)
+          << PreNarrowingValue.getAsString(S.Context, From->getType()) << T;
+
+    return Result;
+  }
+
+  // Check for a narrowing integer conversion.
   bool ReturnPreNarrowingValue = false;
   QualType PreNarrowingType;
   switch (SCS->getNarrowingKind(S.Context, Result.get(), PreNarrowingValue,
diff --git a/clang/test/CXX/drs/dr28xx.cpp b/clang/test/CXX/drs/dr28xx.cpp
index 4d9b0c76758d53..cf9676ddb73385 100644
--- a/clang/test/CXX/drs/dr28xx.cpp
+++ b/clang/test/CXX/drs/dr28xx.cpp
@@ -59,6 +59,41 @@ void B<int>::g() requires true;
 
 } // namespace cwg2847
 
+namespace cwg2851 { // cwg2851: 19
+
+#if __cplusplus >= 202002L
+template<typename T, T v> struct Val { static constexpr T value = v; };
+
+// Floating-point promotions
+static_assert(Val<long double, 0.0>::value == 0.0L);
+static_assert(Val<long double, 0.0f>::value == 0.0L);
+static_assert(Val<double, 0.0f>::value == 0.0);
+static_assert(Val<long double, -0.0>::value == -0.0L);
+static_assert(!__is_same(Val<long double, -0.0>, Val<long double, 0.0L>));
+static_assert(__is_same(Val<long double, 0.5>, Val<long double, 0.5L>));
+static_assert(__is_same(Val<long double, __builtin_nan("")>, Val<long double, __builtin_nanl("")>));
+static_assert(__is_same(Val<long double, __builtin_inf()>, Val<long double, __builtin_infl()>));
+
+// Floating-point conversions where the source value can be represented exactly in the destination type
+static_assert(Val<float, 0.0L>::value == 0.0L);
+static_assert(__is_same(Val<float, 0.0>, Val<float, 0.0L>));
+static_assert(__is_same(Val<float, 0.0>, Val<float, 0.0f>));
+static_assert(!__is_same(Val<float, -0.0L>, Val<float, 0.0f>));
+static_assert(__is_same(Val<float, 0.5L>, Val<float, 0.5f>));
+static_assert(__is_same(Val<float, 0.5L>, Val<float, 0.5f>));
+static_assert(__is_same(Val<float, double{__FLT_DENORM_MIN__}>, Val<float, __FLT_DENORM_MIN__>));
+Val<float, double{__FLT_DENORM_MIN__} / 2.0> _1;
+// since-cxx20-error-re@-1 {{non-type template argument evaluates to {{.+}} which can not be represented in type 'float'}}
+Val<float, static_cast<long double>(__FLT_DENORM_MIN__) / 2.0L> _2;
+// since-cxx20-error-re@-1 {{non-type template argument evaluates to {{.+}} which can not be represented in type 'float'}}
+Val<float, __DBL_MAX__> _3;
+// since-cxx20-error-re@-1 {{non-type template argument evaluates to {{.+}} which can not be represented in type 'float'}}
+static_assert(__is_same(Val<float, __builtin_nan("")>, Val<float, __builtin_nanf("")>));
+static_assert(__is_same(Val<float, __builtin_inf()>, Val<float, __builtin_inff()>));
+#endif
+
+}
+
 namespace cwg2858 { // cwg2858: 19
 
 #if __cplusplus > 202302L

Copy link
Contributor

@Endilll Endilll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the way DR test is written, but I'm not qualified to review the contents on the test, unfortunately.

I'd like to point to related Core issues CWG2836 and CWG2864.

You also need to run clang/www/make_cxx_dr_status to update the DR status page.

@Endilll Endilll requested a review from cor3ntin April 28, 2024 09:07
@MitalAshok MitalAshok force-pushed the cwg2851 branch 2 times, most recently from a87399f to 13e2943 Compare April 28, 2024 13:26
@MitalAshok
Copy link
Contributor Author

Waiting on #90352 because it's not on the cxx_dr_status page yet (make_dr_status doesn't do anything)

Copy link
Contributor

@Sirraide Sirraide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach here looks correct, but I’m not sure it’s in the right place.

clang/lib/Sema/SemaOverload.cpp Show resolved Hide resolved
@Endilll
Copy link
Contributor

Endilll commented Apr 29, 2024

#90352 has been merged.

// Payload is shifted right so these payloads will be preserved
static_assert(__is_same(Val<float, __builtin_nan("0xFF00000000")>, Val<float, static_cast<float>(__builtin_nan("0xFF00000000"))>));
static_assert(__is_same(Val<float, __builtin_nans("0xFF00000000")>, Val<float, static_cast<float>(__builtin_nans("0xFF00000000"))>));
static_assert(__is_same(Val<float, __builtin_nanl("0x1")>, Val<float, static_cast<float>(__builtin_nanl("0x1"))>));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does nanl fail but nans does not? I am not sure this is consistent with CWG2864, which is not final yet either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how exactly we plan to deal with NaN. How APFloat::convert seems to work is that when converting to a smaller type, only the most significant bits of the payload are used, so that 1 is cut off (thus lossy).

This has the side effect that Val<float, (long double) __builtin_nansf(payload)> and Val<float, (long double) __builtin_nanf(payload)> always work, because (long double) __builtin_nanf(payload) is like __builtin_nanl(payload << (difference in number of fraction bits between long double and float)), and anything with the lower bits set counts as "lossy". Anything implicitly converted to a larger type can be converted back in a converted constant expression. See also: IEEE754 6.2.3 "NaN propagation"

CWG2864 is about narrowing conversions. For sure float{__builtin_nanl("0x1")} isn't a narrowing conversion because it's not finite, but it does lose the payload entirely (float{__builtin_nanl("0x1")} and float{__builtin_nanl("0x0")} have the same bit pattern, even though the source long doubles don't). But maybe we should consider all NaNs to have the same "value" and this should work?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discussed this with @jensmaurer offline and he felt similarly as well. So let's go with that for now but I will make sure we discuss that next we discuss cwg2864.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants