[PATCH] D80836: Support GCC [[gnu::attributes]] in C2x mode

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.

In D80836#2064182 <https://reviews.llvm.org/D80836#2064182>, @dblaikie wrote:

> (sorry @erichkeane didn't mean to usurp your feedback by approving before it was answered - @aaron.ballman do treat my approval as contingent on that feedback being addressed as you/both see fit)


No problem :)  The "KnownToGCC" issue is likely a separate issue that should be fixed in a separate patch.

The source-of-the-not-supported-by-C info is simply "if you got this elsewhere, please document the source for future reference", which can be handled without me double checking.  If its self experimentation, that makes me sad, but I don't think there is further action that could be taken about that one.



================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:87
       Ret.emplace_back("CXX11", std::string(Name), "gnu", true);
+      if (Spelling->getValueAsBit("AllowInC"))
+        Ret.emplace_back("C2x", std::string(Name), "gnu", true);
----------------
rsmith wrote:
> erichkeane wrote:
> > I guess its a problem for all of these, but why is the last 'true'/'false' (KnownToGCC) not checked from the Spelling?  It has:  let KnownToGCC = 1;.
> > 
> > I would presume that line either is meaningless, or should be used for the true/false bits in here.
> The `KnownToGCC` flag affects whether we produce certain warnings, not which spellings we register. This does seem fishy: we have the same information represented and examined both by considering whether the attribute has a `GNU` vs `GCC` spelling and by considering whether the `KnownToGCC` flag is set. I imagine we could factor this better. (The two concerns are, I suppose, notionally orthogonal, but I can't imagine we would ever want them to differ.)
Well, both are used to set the value in "FlattenedSpelling".  Here we use true/false, but if you construct one via a record it uses the KnownToGCC flag.  It seems to me that these values should be initialized consistently.

The "spelling" Variety themselves I can see being different, but for consistency's sake, these trues in the emplace_back should set KnownToGCC with the attribute.  That, or we abandon the KnownToGCC 'Value' in the spelling and just always infer it based on the variety.

Like I said, its just weird that sometimes we set this via the tablegen file, sometimes we do it automagically.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80836/new/

https://reviews.llvm.org/D80836