arphaman 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") {
----------------
arphaman wrote:
> aaron.ballman wrote:
> > 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).
> > 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.
>
> I'm not sure I understand how Clang should pass that information to the GNU
> spelling. Here we already set Version in each spelling. Do you think "Clang"
> should inherit from "GNU" spelling in tablegen, e.g.:
>
> ```
> class Clang<string name, bit allowInC = 1, int version = 1>
> : GNU<name, "Clang", version> {
> bit AllowInC = allowInC;
> }
> ```
@aaron.ballman @erichkeane I updated this function to use FlattenedSpelling -
now I don't have to check for "Clang" explicitly. How does it look now.
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
