aaron.ballman added inline comments.
================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:3318
+ for (const auto &Spelling : Attr->getValueAsListOfDefs("Spellings")) {
+ if (Spelling->getValueAsString("Variety") == Variety ||
+ Spelling->getValueAsString("Variety") == "Clang") {
----------------
erichkeane wrote:
> arphaman wrote:
> > erichkeane wrote:
> > > Why is this =="Clang" specific? Since you've added the Version to the
> > > spelling, I'd anticipate us to just be able to grab it for the current
> > > spelling. I wouldn't want an individual spelling here to override it,
> > > particularly since with this change Clang could potentially override the
> > > standards version.
> > I needed it since there's no specific "Clang" variety that's being called
> > for this function. Otherwise the "GNU" variety passed to the function
> > doesn't match "Clang" variety in the record. What's the best way to compute
> > the current spelling in this case?
> Hmm... that is strange. I would have expected this to be called for each of
> the individual spellings, it doesn't really make sense that it wouldn't pick
> it up from the base 'Spellings'. I've unfortunately not debugged this code
> generation in a while. BUT, we care about the 'version' on a per-spelling
> basis, so it would presumably need to 'pick' an actual spelling.
This does get called for each of the base spellings:
https://github.com/llvm/llvm-project/blob/main/clang/utils/TableGen/ClangAttrEmitter.cpp#L3393
I think the issue is that, in Attr.td, the `GNU` spelling has no version
information associated with it. I think what should happen is that spelling
should get a `Version` field the same as `CXX11` and `C2X` do, and the `Clang`
spelling can pass that information along to the `GNU` spelling so that it gets
picked up here during tablegen.
One thing we should probably be careful of is compatibility with GCC. I *think*
GCC just returns 0 or 1 from `__has_attribute` but we should find out if they
return specific values for any of the attributes we already support (a
follow-up patch can correct those values so we match GCC if necessary).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141324/new/
https://reviews.llvm.org/D141324
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
