aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
================
Comment at: include/clang/ASTMatchers/Dynamic/VariantValue.h:335
unsigned Unsigned;
+ double Double;
bool Boolean;
----------------
Lekensteyn wrote:
> aaron.ballman wrote:
> > Lekensteyn wrote:
> > > 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).
> > > The strtod conversion was based on parseDouble in
> > > lib/Support/CommandLine.cpp, so any conversion issues also exist there.
> >
> > Good to know.
> >
> > > Same question, can APFloat/APValue be used in a union?
> >
> > I believe so, but I've not tried it myself. Also, as I mentioned, `APValue`
> > demonstrates another implementation strategy in case you cannot use a union
> > directly.
> >
> > > 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).
> >
> > That's reasonable for an initial implementation.
> I think I'll keep it like this for now and defer eventual conversion to
> APValue for a future patch that also makes uint64_t possible. Is that OK?
Okay, that seems reasonable to me.
https://reviews.llvm.org/D33135
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
