jcranmer-intel added a comment.
So while trying to review this patch, I've discovered there's an annoying
incompatibility between C and C++ here, in that C and C++ specify different
rules on how to choose between `_Float64`, `double`, and `long double` if all
are `binary64` (C says `_Float64` > `long double` > `double`, C++ says `long
double` > `_Float64` > `double`), and I don't have enough knowledge of clang
internal implementation to know if the changes being done can capture the
different semantics there. (It's also not entirely clear to me that the
incompatibility between C and C++ was intentional in the first place; it looks
like the paper author intentionally chose the ordering for C++ and argued for
the change to C, but the CFP working group soundly rejected the change, and
personally I agree with the CFP decision here over C++).
================
Comment at: clang/lib/AST/ASTContext.cpp:136-138
+// another floating point type.
+constexpr std::array<std::array<FloatingRankCompareResult, 5>, 5>
+ CXX23FloatingPointConversionRankMap = {
----------------
I don't like having a large table here of results. It just doesn't scale; if
you take into account all of the putative types that might be supported, you've
got 3 basic types + 4 interchange formats + 3 decimal types + 1 IEEE extended
type + bfloat + 3 IBM hex floats, and that's just things already supported by
LLVM or that I know someone's working on.
I think after special casing float/double/long double, you can handle all the
other types by simply using a mixture of `fltSemantics::isRepresentableBy` and
a subrank preference list.
In the event that support for `_Float32` and `_Float64` are added, this table
will no longer be target-independent. We have a few targets where `double` is
binary32 and several targets where `long double` is either binary32 or binary64
(and others where it's neither). I think it's better to start writing this
method in a way that can handle this mapping be target-dependent.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149573/new/
https://reviews.llvm.org/D149573
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
