[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs added inline comments.


================
Comment at: clang/lib/Sema/SemaCast.cpp:1367
+      Self.Context.doCXX23ExtendedFpTypesRulesApply(DestType, SrcType)) {
+    // Support for cast between fp16 and bf16 doesn't exist yet.
+    if (!((DestType->isBFloat16Type() || DestType->isFloat16Type()) &&
----------------
tahonermann wrote:
> Should this be a FIXME comment? What happens in this case? Should we perhaps assert on such attempted conversions?
I have added the FIXME. There is a test case for this scenario:

```_Float16 f16_val_7 = static_cast<_Float16>(1.0bf16); // expected-error {{static_cast from '__bf16' to '_Float16' is not allowed}}```

If you still believe it might be better to add an assert I can do that, please let me know.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1235-1237
+    if (order == FRCR_Unordered) {
+      return QualType();
+    }
----------------
tahonermann wrote:
> Return of an invalid type is a new behavior for this function and it isn't clear to me that callers will handle it (or be expected to handle it) such that a diagnostic will be generated. Perhaps a diagnostic should be issued here? Or perhaps this should be an assertion?
It results in an error, please see the below test case:

```auto f16_bf16 = 1.0f16 + 1.0bf16; // expected-error {{invalid operands to binary expression ('_Float16' and '__bf16')}}```

This function is only called by `UsualArithmeticConversions` which returns invalid type in several places, hence returning invalid type should be ok? If you still prefer an assertion or diagnostic, I will happily add one.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:4202
+          if (S.Context.getFloatingTypeOrder(SCS2.getToType(1),
+                                             SCS1.getFromType()) < FRCR_Equal) {
+            return ImplicitConversionSequence::Better;
----------------
tahonermann wrote:
> This comparison includes `FRCR_Unordered`, is that intended? (another case at line 4225 below)
Yes, my understanding is we are checking if the second sequence's type's (T3) conversion rank is not equal to the conversion rank of FP2. Is that not correct?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149573/new/

https://reviews.llvm.org/D149573