Lekensteyn added inline comments.
================
Comment at: include/clang/ASTMatchers/Dynamic/VariantValue.h:335
unsigned Unsigned;
+ double Double;
bool Boolean;
----------------
aaron.ballman wrote:
> 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`).
The strtod conversion was based on parseDouble in lib/Support/CommandLine.cpp,
so any conversion issues also exist there.
Same question, can APFloat/APValue be used in a union?
float (or quad-double suffixes) are explicitly not supported now in this
matcher, maybe they can be added later but for now I decided to keep the
grammar simple (that is, do not express double/float data types via the
literal).
================
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;
----------------
aaron.ballman wrote:
> You're failing to check errno here to ensure the value is actually valid.
will check this later, see also the previous comment
https://reviews.llvm.org/D33135
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
