LGTM, thank you!

LGTM, thank you!

(I've not had the chance to complete a full review, but these are some thoughts thus far.)

I've commit in r315856, thank you for the reviews!

Ping.

The attribute and sema bits look good to me, but I agree that you might want Richard's opinions before committing.

Corrected review feedback from Richard.

I've commit in r315057. Thank you!

Adding @dberlin for licensing discussion questions.

In D37436#881363, @aaron.ballman wrote:

In D37436#871117, @aaron.ballman wrote:

Updated based on review feedback.

• Rename CAttributes to DoubleSquareBracketAttributes
• Added Objective-C test cases to demonstrate no regressed behavior (based on a use-case pointed out during review)
• Fixed regression with GNU-style attributes in enumerations
  
  I still need to rename the cc1 flag, pending agreement on the name.

Ping -- the last outstanding issue that I'm aware of is the cc1 flag name, do you have a preference there, Richard? If not, I'll be going with the verbose -fdouble-square-bracket-attributes.

LGTM!

Aside from a small nit, this LGTM, thanks!

LGTM!

LGTM, with a comment about the unrelated test change.

LGTM!

Committed in r314463.

LGTM! Do you need me to commit on your behalf?

In D38342#883872, @Rakete1111 wrote:

• Moved test to test/CXX/
  
  Do I actually need to -verify the test if no diagnostics are expected?
  
  Thanks @aaron.ballman

In D36836#883214, @lebedev.ri wrote:

@alexfh please specify, is that enough or not?

In D38205#882737, @erichkeane wrote:

changes that were suggested by @aaron.ballman

It DOES warn on the template correctly, however I'm not thrilled that it misses the 'warning' info. Any idea how I can get that information to give that 'note'? I'd like an "in instantiation of template ..." type note.

This is somewhat complicated in that noexcept, throw(), and attribute(())/__declspec no throw are all synonyms for one another except for the type system effect differences between attribute and __declspec, but I think our behavior here is reasonable.

LGTM!

The code for printf diagnostics + new tests are supposed to be added by a separate diff.

In D37436#871117, @aaron.ballman wrote:

Updated based on review feedback.

• Rename CAttributes to DoubleSquareBracketAttributes
• Added Objective-C test cases to demonstrate no regressed behavior (based on a use-case pointed out during review)
• Fixed regression with GNU-style attributes in enumerations
  
  I still need to rename the cc1 flag, pending agreement on the name.

In D38209#880553, @STL_MSFT wrote:

do you think __declspec(nothrow) calling the terminate handler in Clang is a bug?

It's certainly a behavior difference with potential performance impact, although I don't think it can be viewed as a bug, strictly speaking. MSVC treats __declspec(nothrow) as an optimization request, with undefined behavior if it's violated. Termination is definitely one of the possible results of undefined behavior.

We've recently had to slightly rethink our EH strategy in light of C++17's addition of noexcept into the type system, and the removal of dynamic exception specifications (and the change in semantics to throw()). MSVC's /EHsc makes this extra fun. If you're interested, I can put you in contact with the compiler dev who recently made those changes.

I'm not certain we have the semantics of __declspec(nothrow) exactly correct -- a simple test shows that __attribute__((nothrow)), __declspec(nothrow), and noexcept(true) are subtly different.

Aside from a minor testing nit, LGTM! Thanks!

Aside from a few minor nits, LGTM!

Thank you for working on this -- it's very nice functionality!

LGTM!

Adding a few reviewers to hopefully help Roman get some feedback.

Updated based on review feedback.

In D37436#870305, @rsmith wrote:

Also, your wording paper appears to allow things like

struct [[foo]] S *p; // ok in c n2137, ill-formed in c++
struct T {};
int n = sizeof(struct [[foo]] T); // ok in c n2137, ill-formed in c++

You don't seem to implement those changes; are they bugs in the wording paper?

In D37436#869851, @hfinkel wrote:

In D37436#869467, @aaron.ballman wrote:

In D37436#869462, @hfinkel wrote:

I think that I misunderstood your concern. Let me see if I can summarize your position: You believe that, when GCC implements this syntax in C, they will audit their attributes and not support all of their existing gnu:: attributes in C. You only want us to support these when we know what that list will be (which we don't yet). Is that correct?

Yes, that is correct.

Okay. A large fraction of the number of attributes we'll want to use are going to fall into this category (because Clang doesn't have its own attributes, but copied GCC's, for many things). I don't think we'll get good implementation feedback until we have this figured out. If we can't sync with GCC soon, I suggest just making a reasonable guess. My first choice would be to just allowing everything, and then we can see what people found to be useful and why. Our experience here can also provide feedback to GCC (and we can always update late if needed - it is still an experimental feature).

In D37436#869462, @hfinkel wrote:

I think that I misunderstood your concern. Let me see if I can summarize your position: You believe that, when GCC implements this syntax in C, they will audit their attributes and not support all of their existing gnu:: attributes in C. You only want us to support these when we know what that list will be (which we don't yet). Is that correct?

Added full context, no other changes from previous patch.

In D37436#869350, @hfinkel wrote:

In D37436#868333, @aaron.ballman wrote:

In D37436#868295, @hfinkel wrote:

In D37436#867965, @aaron.ballman wrote:

In D37436#867287, @rsmith wrote:

If this is just supposed to be an experiment to get feedback on the feature, then I don't think we should be treating it as a different attribute syntax at all. Rather, I think we
just want to permit C++11 attributes to be parsed in other language modes. If/when this becomes part of some future C working draft, I think that's the time to have a
separate attribute syntax with a distinct set of valid unqualified attribute names.

I do not think that's the correct approach. These are not C++ attributes (for instance, no using insanity is allowed, :: is a new lexing token in C, etc). Also, I don't think it's a good idea to enable all C++11-style attributes in C mode without giving each attribute some appropriate thought (what does abi_tag *do* in C mode? What happens with _Noreturn vs [[noreturn]] etc). Also, I'm not comfortable adding a bunch of vendor-specific gnu:: attributes that GCC does not implement in C yet.

On this last point, I disagree. Implementation experience is about all of the messy things that occur in production. In production, if we have this syntax, then we'll end up enabling it for a bunch of vendor-specific attributes. Do you think that we wouldn't?

I'm sure we would. Also, FWIW, I was planning to traverse the attributes we implement to find which clang-specific C++ attributes would make sense to also enable as a follow-up patch once the syntax is in.

N2137 specifically talks about this as a use case. If so, this will include gnu:: attributes that we have in Clang (even if GCC does not implement them).

Eventually, yes, but it seems like a problem to implement something under that vendor namespace when the vendor themselves do not. I think it would be really unfortunate were GCC to add a C++ attribute named [[clang::frobble]] that Clang does not implement, and I don't see this case as being all that different. My belief is that GCC will eventually elect to make most of these attributes available in C mode and that's an appropriate time for us to do the same for their vendor namespace.

From my perspective, lack of consistency here between Clang's C and C++ modes is much more problematic than a lack of consistency between what Clang and GCC implement.

From my perspective, they're both problems in their own right. To me (and maybe I'm weird with this line of reasoning), the only reasonable time to implement an attribute under another vendor's attribute namespace is when you are promising your users that you will attempt to match the owning vendor's semantics for that attribute. A case could be made here that the owning vendor *has* implemented that attribute (since they have in C++), but I'm not too comfortable *assuming* that the GCC folks are okay with this since they don't implement the feature syntax in C yet.

That said, I'm happy to ask Jason at the meetings in Albuquerque to explore the idea -- but I don't think it should hold up this patch, especially since we have our own vendor attributes we can use for gaining experience.

I certainly understand your perspective, but this is an orthogonal concern. If this is something that Clang does, then it should do it consistently. If you'd like us not to support gnu:: attributes that GCC itself does not support, and that's something that we currently do in C++, then please submit a patch to fix that for all language modes. It should not differ between language modes.

Is the problem here that we're treating gnu::, not as a vendor prefix, but as generic escape hatch to get to anything generally provided via GCC-attribute syntax (which many compilers, including ours, have extended with attributes that GCC does not itself support)?

In D37436#868295, @hfinkel wrote:

In D37436#867965, @aaron.ballman wrote:

In D37436#867287, @rsmith wrote:

If this is just supposed to be an experiment to get feedback on the feature, then I don't think we should be treating it as a different attribute syntax at all. Rather, I think we
just want to permit C++11 attributes to be parsed in other language modes. If/when this becomes part of some future C working draft, I think that's the time to have a
separate attribute syntax with a distinct set of valid unqualified attribute names.

I do not think that's the correct approach. These are not C++ attributes (for instance, no using insanity is allowed, :: is a new lexing token in C, etc). Also, I don't think it's a good idea to enable all C++11-style attributes in C mode without giving each attribute some appropriate thought (what does abi_tag *do* in C mode? What happens with _Noreturn vs [[noreturn]] etc). Also, I'm not comfortable adding a bunch of vendor-specific gnu:: attributes that GCC does not implement in C yet.

On this last point, I disagree. Implementation experience is about all of the messy things that occur in production. In production, if we have this syntax, then we'll end up enabling it for a bunch of vendor-specific attributes. Do you think that we wouldn't?

In D33826#868167, @lebedev.ri wrote:

In D33826#868085, @aaron.ballman wrote:

In D33826#866170, @lebedev.ri wrote:

In D33826#866161, @JonasToth wrote:

There is an exception to the general rule (EXP36-C-EX2), stating that the result of malloc and friends is allowed to be casted to stricter alignments, since the pointer is known to be of correct alignment.

Quote for the reference:

EXP36-C-EX2: If a pointer is known to be correctly aligned to the target type, then a cast to that type is permitted. There are several cases where a pointer is known to be correctly aligned to the target type. The pointer could point to an object declared with a suitable alignment specifier. It could point to an object returned by aligned_alloc(), calloc(), malloc(), or realloc(), as per the C standard, section 7.22.3, paragraph 1 [ISO/IEC 9899:2011].

For plain calloc(), malloc(), or realloc(), i would guess it's related to max_align_t / std::max_align_t / __STDCPP_DEFAULT_NEW_ALIGNMENT__, which is generally just 16 bytes.

It's a requirement from the C standard that malloc, calloc, and realloc return a suitably-aligned pointer for *any* type.

We are probably arguing about the details, or i'm just misguided, but you mean any *standard* type, right?

MALLOC(3)
 ...
 The  malloc()  and calloc() functions return a pointer to the allocated memory, which is suitably aligned for any built-in type.

E.g. __m256 needs to be 32-byte aligned, and user type, with the help of alignas(), can have different alignment requirements

Good catch on the deleted constructor -- LGTM still!

LGTM, with a few last typos.

In D33826#867155, @rjmccall wrote:

In D33826#866170, @lebedev.ri wrote:

In D33826#866161, @JonasToth wrote:

There is an exception to the general rule (EXP36-C-EX2), stating that the result of malloc and friends is allowed to be casted to stricter alignments, since the pointer is known to be of correct alignment.

In practice, are there any malloc declarations still in use that return char* instead of void*? I assume this does not complain when the source is void*.

In D33826#866170, @lebedev.ri wrote:

In D33826#866161, @JonasToth wrote:

There is an exception to the general rule (EXP36-C-EX2), stating that the result of malloc and friends is allowed to be casted to stricter alignments, since the pointer is known to be of correct alignment.

Quote for the reference:

EXP36-C-EX2: If a pointer is known to be correctly aligned to the target type, then a cast to that type is permitted. There are several cases where a pointer is known to be correctly aligned to the target type. The pointer could point to an object declared with a suitable alignment specifier. It could point to an object returned by aligned_alloc(), calloc(), malloc(), or realloc(), as per the C standard, section 7.22.3, paragraph 1 [ISO/IEC 9899:2011].

For plain calloc(), malloc(), or realloc(), i would guess it's related to max_align_t / std::max_align_t / __STDCPP_DEFAULT_NEW_ALIGNMENT__, which is generally just 16 bytes.

In D37436#867287, @rsmith wrote:

If this is just supposed to be an experiment to get feedback on the feature, then I don't think we should be treating it as a different attribute syntax at all. Rather, I think we
just want to permit C++11 attributes to be parsed in other language modes. If/when this becomes part of some future C working draft, I think that's the time to have a
separate attribute syntax with a distinct set of valid unqualified attribute names.

Updates based on review feedback.

Updated based on Richard's comments and some further discussion on IRC.

LGTM, thanks!

LGTM!

Please run the test through clang-format, but otherwise LGTM!

The bulk of the patch LGTM as well, but it should still have a test case.

In D37620#864684, @sammccall wrote:

Thanks!
Unless I'm missing something, the current patch just adds the category, but doesn't actually recategorize the warning, right?

Does this need test?

Have you run this check over any large code bases to see what the quality of the diagnostics are?

Aside from a few small nits, this looks reasonable. Have you run it over any large code bases that use signals to test the quality of the check?

Assuming no sphinx issues with the docs, this LGTM, thank you!

LGTM! Thank you for working on this!

In D37308#858035, @zahiraam wrote:

The test case you gave doesn't compile. It returns the diagnostic. CL has the same behavior. I don't think it is because of the dllimport.

In D37308#857607, @zahiraam wrote:

Removed the helper function.

If RD (base class) has uuid attribute, we want to ensure that the interface doesn't have attributes. Otherwise cases like:

class declspec(uuid("00000000-0000-0000-C000-000000000046")) IUnknown1 {};
interface __declspec(dllimport) ISfFileIOPropertyPage1 : public IUnknown1 {};

will compile.

LGTM!

In D36272#857005, @anatol.pomozov wrote:

Hi Aaron. Thank you very much for reviewing this patch.

I'll be glad to add documentation for 'force_align_arg_pointer' attribute and upload another patch. But before doing it I would like to finish work on the current patch and make its forward progress toward repository.