aaron.ballman added a comment.
In https://reviews.llvm.org/D33135#754278, @Lekensteyn wrote:
> By the way, I think that `long double` is less common than long unsigned
> literals, so changing unsigned to uint64_t might be something more important?
I agree that it's likely a more common use case. There doesn't appear to be a
suffix for __int128 (we talked about adding i128 once upon a time, but I don't
believe it got in), so you may be okay using a `uint64_t` there rather than an
`APInt`.
================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:25
/// <Boolean> := true | false
+/// <Double> := 1.0 | 2e-3 | 3.45e67
/// <Unsigned> := [0-9]+
----------------
Lekensteyn wrote:
> aaron.ballman wrote:
> > It'd be good to list the actual grammar rather than a few examples.
> I am concerned that listing a very precise grammar actually makes this less
> readable (see also the StringLiteral example).
>
> If a grammar is used instead, how about this:
>
> <Double> := [0-9]+.[0-9]* | [0-9]+.[0-9]*[eE][-+]?[0-9]+
>
That's reasonable enough.
================
Comment at: include/clang/ASTMatchers/Dynamic/VariantValue.h:335
unsigned Unsigned;
+ double Double;
bool Boolean;
----------------
Lekensteyn wrote:
> aaron.ballman wrote:
> > This may or may not be a good idea, but do we want to put the values into
> > an APFloat rather than a double? My concern with double is that (0) it may
> > be subtly different if the user wants a 16- or 32-bit float explicitly, (1)
> > it won't be able to represent long double values, or quad double.
> >
> > I'm thinking this value could be passed directly from the C++ API as an
> > APFloat, float, or double, or provided using a StringRef for the dynamic
> > API.
> (32-bit) double values are a superset of (16-bit) float values, that should
> be OK.
> Long doubles are possible in the AST (e.g. for `0.1L`), but neither C11 nor
> C++14 seem to define a quad double literal type (so that should be of a
> lesser concern).
>
> Reasons why I chose for double instead of APFloat:
> - `strtod` is readily available and does not abort the program. By contrast,
> `APFloat(StringRef)` trips on assertions if the input is invalid.
> - I was not sure if the APFloat class can be used in an union.
The downside to using `strtod()` is that invalid input is silently accepted.
However, assertions on invalid input is certainly not good either. It might be
worth modifying `APFloat::convertFromString()` to accept invalid input and
return an error.
I think instead of an `APFloat`, maybe using an `APValue` for both the
`Unsigned` and `Double` fields might work. At the very least, it should give
you implementation ideas.
There is a quad double literal suffix: `q`. It's only supported on some
architectures, however. There are also imaginary numbers (`i`) and half (`h`).
================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:180
/// \brief Consume an unsigned literal.
void consumeUnsignedLiteral(TokenInfo *Result) {
+ bool isFloatingLiteral = false;
----------------
Lekensteyn wrote:
> aaron.ballman wrote:
> > This function should be renamed and the comment updated.
> How does "consumeNumberLiteral" sound?
Sounds good to me.
================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:209
+ double doubleValue = strtod(Result->Text.str().c_str(), &end);
+ if (*end == 0) {
+ Result->Kind = TokenInfo::TK_Literal;
----------------
You're failing to check errno here to ensure the value is actually valid.
https://reviews.llvm.org/D33135
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
