aaron.ballman added inline comments.
================ 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: > > > > 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. https://reviews.llvm.org/D33135 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits