Many `Clang` spelling attributes do not test the `[[]]` spelling

While working on ⚙ D126061 [clang] [WIP] Reject non-declaration C++11 attributes on declarations, I noticed that many attributes that are declared in Attr.td with a Clang spelling do not test the the [[]] spelling of the attribute. Just one example is noderef, which currently in fact does not work correctly in the [[]] spelling.

The documentation for the the Clang spelling says these things:

  • “The Clang spelling implies GNU, CXX11<“clang”, name>, and optionally, C2x<“clang”, name>”.
  • “This spelling should be used for any Clang-specific attributes”.

What I suspect is happening is that some authors intend to introduce a Clang-specific attribute that only uses the __attribute__ spelling. They see the guidance “this spelling should be used for any Clang-specific attributes” and therefore declare their attribute with a Clang spelling, but only add tests for the __attribute__ spelling because they don’t actually want the [[]] spelling.

I think one of two things would need to happen:

  • If Clang intends to support Clang-specific attributes with only an __attribute__ spelling, there should be a separate Spelling for this purpose.

  • If Clang intends all Clang-specific attributes to support the [[]] spelling, code reviewers should make sure that authors include tests for this spelling.

1 Like

Yes and no. I have a policy that any new attribute that’s unique to Clang is spelled with the Clang spelling unless there’s a reason not to (like, not being able to use it due to positioning issues, like we ran into with diagnose_if). So most often, what happens is that during review, I ask why an attribute which is unique to Clang is limited to only one spelling. “This spelling should be used for any Clang-specific attributes” is intended to apply to an attribute which is specific to Clang (as opposed to one which is a GCC compatibility attribute, which uses the GCC spelling).

If Clang intends to support Clang-specific attributes with only an __attribute__ spelling, there should be a separate Spelling for this purpose.

I’d like that to continue to be spelled GNU or Declspec (or whatever individual spelling makes sense). The GCC and Clang spellings exist so that you can put the same attribute name into multiple spellings with the appropriate vendor scope.

If Clang intends all Clang-specific attributes to support the [[]] spelling, code reviewers should make sure that authors include tests for this spelling.

I agree with this, but it would be nice if we had some machinery to make that testing easier on patch authors. Having to duplicate a significant number of tests just to test spelling variants that should be semantically identical between spellings is an uphill battle. Most attributes should be semantically identical between spellings, so I’m wondering if we can narrow the testing request down to just situations that have been problematic (like type attributes)? If we can, then perhaps we can come up with some testing machinery to help – macros to help in lit tests, or start adding unit testing infrastructure that can test multiple spellings from one code snippet, etc.

1 Like

Thanks for the clarification!

I’m probably biased because I have merely been looking at type attributes. Also, just to clarify, I wasn’t trying to suggest that all tests should be duplicated in all possible spellings – that would indeed be overly onerous. I think a few judicious “spot checks” in the alternative spellings should generally be enough.

Heh, there’s sort of a hierarchy of understanding when it comes to attributes. We have a very good handle on declaration attributes for the most part, statement attributes are a wee bit less well-handled than declaration attributes (not as much automated machinery, nowhere near as many attributes), but type attributes are basically the Wild West (almost no automated machinery, most type attributes are slightly different from other type attributes, etc). So it’s not a huge surprise that our testing support for type attributes is underwhelming.

Ah, I definitely agree that would be helpful. I’ll keep it in mind when doing future reviews, thank you!

1 Like