zahiraam added a comment. In D146148#4221651 <https://reviews.llvm.org/D146148#4221651>, @rjmccall wrote:
> In D146148#4220591 <https://reviews.llvm.org/D146148#4220591>, @zahiraam > wrote: > >> In D146148#4220495 <https://reviews.llvm.org/D146148#4220495>, >> @aaron.ballman wrote: >> >>> In D146148#4220433 <https://reviews.llvm.org/D146148#4220433>, @zahiraam >>> wrote: >>> >>>> In D146148#4220268 <https://reviews.llvm.org/D146148#4220268>, @rjmccall >>>> wrote: >>>> >>>>> Okay, so modifying `math.h` to use this attribute is acceptable? That's >>>>> great, that's definitely the best outcome for the compiler. >>>>> >>>>> Just a minor request about the diagnostic name, but otherwise LGTM. >>>> >>>> Yes. Added the attribute inside the included math.h header file. >>> >>> How does this work for, say, glibc, musl, or MSVC CRT? Won't those math.h >>> headers lack the attribute and thus run into problems when used with Clang? >> >> Good point! @rjmccall are you thinking of something in particular with the >> attribute? > > Zahira, this is what I was asking you when I asked whether modifying the > math.h header was acceptable: whether you were prepared to accept that the > warning would only fire on system math.h headers that we'd modified, or > whether you cared about making it work with non-cooperative headers. I > wasn't asking if you were willing to change the test code. Okay sorry about the confusion. I think the diagnostic should fire when the user is including system's math.h and using float_t inside a scope cotaining a #pragma clang fp eval-method. I am not sure what you mean by "cooperative headers"? >> If not I guess we will have to rely on string comparison for all the typedef >> in the TU. Aaron pointed out the compile time overhead. > > Well, the compile-time overhead of doing this on every typedef *declaration* > is way better than the overhead of doing this on every type lookup, at least. > > Anyway, there are three possibilities I can see: > > - We accept that this needs cooperative system headers. Not sure what you mean by cooperative system headers. > - We add a math.h compiler header that `#include_next`s the system math.h and > then adds the attribute. I believe you can just add an attribute to a > typedef retroactively with something like `typedef float_t float_t > __attribute__((whatever))`. So when a user writes: #include <math.h> int foo() { #pragma clang fp eval_method(source) return sizeof(float_t); } It would be as though the user wrote: #include "math.h" int foo() { #pragma clang fp eval_method(source) return sizeof(float_t); } where the content of this new math header being: #include_next <math.h> typedef float_t float_t __attribute__((whatever)); typedef double_t double_t __attribute__((whatever)); The compiler would have to recognize that the included file is math.h and create a new math.h. Where would this file reside? > - We do checks on every typedef declaration. There's a builtin-identifier > trick that we do with library functions that we should be able to generalize > to typedefs, so you wouldn't need to actually do string comparisons, you'd > just check whether the `IdentifierInfo*` was storing a special ID. We'd set > that up in `initializeBuiltins` at the start of the translation unit, so the > overhead would just be that we'd create two extra `IdentifierInfo*`s in every > TU. > > The builtin ID logic is currently specific to builtin *functions*, and I > don't think we'd want to hack typedef names into that. But the same storage > in `IdentifierInfo*` is also used for identifying the ObjC context-sensitive > keywords, by just offsetting the builtin IDs by the NUM_OBJC_KEYWORD. You > should be able to generalize that by also introducing a concept of a builtin > *type* name, so that e.g. IDs in [0,NUM_OBJC_KEYWORDS) are ObjCKeywordKinds, > IDs in [NUM_OBJC_KEYWORDS, NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES) are > BuiltinTypeKinds (when you subtract NUM_OBJC_KEYWORDS), and IDs in > [NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES,∞) are builtin function IDs (when you > subtract NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES). Need to look at this more closely to understand what you are suggesting. @rjmccall do you have a preference for any one of these solutions? @aaron.ballman ? Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146148/new/ https://reviews.llvm.org/D146148 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
