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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

This is looking good to me. I added a few comments seeking clarification.

I'm concerned about the changes to the existing calls to `getFloatingTypeOrder()`. The changes look like they will preserve prior behavior for standard types just fine, but I'm not sure whether they implement the right behavior for the extended types. Defining comparison functions that are explicit in their handling of `FRCR_Unordered` and subrank comparisons would help me feel more confident about the changes, the intent, and my ability to audit for the desired behavior.



================
Comment at: clang/include/clang/Lex/LiteralSupport.h:75
   bool isBitInt : 1;        // 1wb, 1uwb (C2x)
+  bool isBF16 : 1;          // 1.0bf
   uint8_t MicrosoftInteger; // Microsoft suffix extension i8, i16, i32, or i64.
----------------
codemzs wrote:
> tahonermann wrote:
> > Is this for `__bf16` or for `std::bfloat16_t`?
> Its for `std::bfloat16_t`, I don't believe `__bf16` has a literal suffix. 
I guess it does now! :)


================
Comment at: clang/lib/AST/ASTContext.cpp:126-127
+    return 4;
+  default:
+    llvm_unreachable("Not a CXX23+ floating point builtin type");
+  }
----------------
Assuming there is no need to handle `__float128` and `__ibm128` here, how about a comment to state why they don't require support here?


================
Comment at: clang/lib/Sema/Sema.cpp:706
+    auto conversionRank = Context.getFloatingTypeOrder(TypeTy, ExprTy);
+    if (conversionRank < FRCR_Greater) {
+      Diag(E->getExprLoc(),
----------------
This comparison depends on the order of the enumerators in `FloatingRankCompareResult` and that seems brittle. I suggest either encompassing such comparisons in a function or, at least, adding a comment to the enumeration definition that explains the ordering requirements.


================
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()) &&
----------------
Should this be a FIXME comment? What happens in this case? Should we perhaps assert on such attempted conversions?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1235-1237
+    if (order == FRCR_Unordered) {
+      return QualType();
+    }
----------------
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?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:4202
+          if (S.Context.getFloatingTypeOrder(SCS2.getToType(1),
+                                             SCS1.getFromType()) < FRCR_Equal) {
+            return ImplicitConversionSequence::Better;
----------------
This comparison includes `FRCR_Unordered`, is that intended? (another case at line 4225 below)


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

https://reviews.llvm.org/D149573