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

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

codemzs added a comment.

@tahonermann I would like to understand your concern better on unordered floating point types as the callers of `getFloatingTypeOrder` handle this result as per the C++23 proposal, for example there is a test case that exercises this scenario: _Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type '__bf16'}}

Perhaps if you can please give me an example that further illustrates your concerns it would help me understand and assuage your concerns. Thank you.



================
Comment at: clang/lib/AST/ASTContext.cpp:191
+           FloatingRankCompareResult::FRCR_Greater,
+           FloatingRankCompareResult::FRCR_Equal}}}};
+
----------------
tahonermann wrote:
> I just realized that none of these comparisons results in `FRCR_Equal_Lesser_Subrank` or `FRCR_Equal_Greater_Subrank`. I guess those won't actually be used until support for `std::float32_t` and `std::float64_t` are added. That means that we don't have a way to test code that performs subrank checks though.
That is correct, my plan was to add support for these types after this change is committed. 


================
Comment at: clang/lib/Frontend/InitPreprocessor.cpp:461
+      Builder.defineMacro("__STDCPP_FLOAT16_T__", "1");
+      Builder.defineMacro("__STDCPP_BFLOAT16_T__", "1");
+    }
----------------
tahonermann wrote:
> Should the definition of `__STDCPP_BFLOAT16_T__` be conditional on something like `Ctx.getTargetInfo().hasFullBFloat16Type()`?
Based on [our discussion](https://discourse.llvm.org/t/rfc-c-23-p1467r9-extended-floating-point-types-and-standard-names/70033/34?u=codemzs) on the RFC it was determined that representing `bfloat16` using `fp32` is ok as per the author of the proposal, in that case do you believe we should be making this macro conditional on the target natively supporting `bfloat16`?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10451
   return ICE->getCastKind() == CK_FloatingCast &&
-         S.Context.getFloatingTypeOrder(From, To) < 0;
+         S.Context.getFloatingTypeOrder(From, To) == FRCR_Lesser;
 }
----------------
tahonermann wrote:
> I'm not sure this is right. If I understand correctly, the C++23 extended FP types don't participate in argument promotions. Perhaps they should be excluded here.
Rules for [promotion](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1467r9.html#promotion) are unchanged as per the proposal. This is just using the new enumeration to represent a smaller conversion rank. 


================
Comment at: clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp:3-6
+_Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type '__bf16'}}
+_Float16 f16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'float'}}
+_Float16 f16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'double'}}
+_Float16 f16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'long double'}}
----------------
tahonermann wrote:
> Are these errors correct? Since the source is a constant expression that specifies a value that is within the representable range of the type, I think these initializations are expected to be allowed.
I believe this should be an error unless my understanding is incorrect, below is what the proposal [says](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1467r9.html#implicit-constant) for constant expressions:

"A drawback of this part of the proposal is that constant values don’t get any special treatment. As a result, this code:

`std::float16_t x = 1.0;`

would be ill-formed. The constant 1.0 has type double, which cannot be implicitly converted to type std::float16_t, even though the value 1.0 can be represented exactly in both types. To compile, the code must have an explicit cast, or, preferably, use a literal suffix:
`std::float16_t x = 1.0f16;`
...."



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

https://reviews.llvm.org/D149573