FWIW, it looks like tests are still failing on Windows according to the CI pipeline.

LGTM, thanks!

Thank you for this patch, I think it's really useful functionality for plugin authors!

Mostly just nits from me, but the attribute portions look good to me.

This continues to LGTM, so I'm accepting it. If @alexfh has any remaining concerns, hopefully he can raise them quickly or we can handle them post-commit. Thanks!

LGTM!

In D75844#2677457, @brooksmoses wrote:

FWIW, this now causes Clang to produce an error on this code, when it didn't before:

using namespace::foo __attribute__((deprecated("message")));

I discussed this with Richard Smith, who points out that GCC does not accept this and it's not permitted according to the C++ standard, so this should probably be considered an accidental fix of a longstanding bug. With that said, would it be useful to add this as a tested case?

Thank you for the patch! Btw, can you add more context to the patch when you generate it (I usually use -U 999 when making patches)?

In D97653#2593287, @njames93 wrote:

Where would be a good place for testing refersToDefaultCapture support?

In D99825#2671963, @xbolva00 wrote:

Unlike in C, in C++, flowing off the end of a non-void function other than main results in undefined behavior even when the value of the function is not used.

LGTM, thank you!

I think the behavior is correct, but I do think the original diagnostic was somewhat better -- an attribute list *can* appear there, if you're lucky. Of course, the old diagnostic isn't great either because adding a ; after the attribute isn't going to improve the situation for the user. So I think I'm actually fine either way, but maybe @rsmith has a preference.

I've commit in 9711118d2edf7aed133616de1eb7f633c263c4b5, thanks for the really quick reviews!

In D99896#2669471, @erichkeane wrote:

It is really sad that the attributes can't 'auto transform' themselves. ParsedAttr could (since it has a union of expressions/identifiers), but we don't really seem to have a good way to do it for Attr.

Could we possibly do some table-gen to auto create the 'simple' ones? Basically, any parameter to an attribute that is an 'expr' gets auto-transformed? Similar to how we have handleSimpleAttr, we could have TransformSimpleAttr (or, a generated one).

Adding a few more reviewers for early feedback.

In D75844#2668095, @thakis wrote:

The test fails on macOS: http://45.33.8.238/macm1/6821/step_6.txt

Please take a look and revert for now if it takes a while to fix.

Thank you for the additional test case, this LGTM again.

Committed in 4be8a26951da9a6e04de327b38dd158f4c8e3280

Accepting on my own authority so I can close in a moment.

Correcting lint warnings, adding documentation for the new feature.

@rsmith -- I am posting the review so that I can see how the CI pipeline likes the functionality (because this impacts so many different attributes), but I intend to commit it on my own authority once CI passes. Feel free to review it if you'd like and I'll address any concerns you have post-commit.

The CI is showing build failures and there are some clang-tidy nits to be addressed as well.

In D75844#2664119, @tbaeder wrote:

Damn. Reverted again for the time being. The libc++ build seems to fail and I won't have time to look into that this week. :(

LGTM!

Thank you for picking this back up!

LGTM, thank you for the fixes!

LGTM, thank you for the patch!

Generally LGTM but I did have a question about the feature testing macro.

Thanks for merging, I think this seems reasonable but is missing some test coverage still.

In D99278#2657664, @tbaeder wrote:

This (and https://reviews.llvm.org/D99338) are both NFC changes once https://reviews.llvm.org/D97362 lands (they need the three-parameter version of ProhibitCXX11Attributes()). I can merge the three into one patch if you prefer.

LGTM aside from the test case commenting nit. Thank you!

LGTM with a minor improvement for attribute ordering.

Mostly looks good, just a few things with the tests.

To be clear, this is expected to be an NFC change that allows D97362 to be applied without breaking bots? If so, it'd be helpful to have the changes as part of D97362 because they're critical to that review and so that we get appropriate CI pre-merge testing.

To be clear, this is expected to be an NFC change that allows D97362 to be applied without breaking bots? If so, it'd be helpful to have the changes as part of D97362 because they're critical to that review and so that we get appropriate CI pre-merge testing.

Mostly grammar related review comments, nothing substantive.

Thanks for working on this! I think the direction is good in general, but I think we should also add tests for use in the preprocessor (#if 1z == 1, etc) as well as tests for the behavior in C.

In D73967#2653042, @keryell wrote:

An FPGA programmer is hitting this issue from your unit test:

c++
  signed _ExtInt(1) m; // expected-error{{signed _ExtInt must have a bit size of at least 2}}

Why do you not allow a type able to represent {-1, 0}?

In D82880#2651698, @davidstone wrote:

In D82880#2651219, @aaron.ballman wrote:

Do you need me to commit on your behalf? If so, which email address would you like me to use for attribution?

Yeah, I do not have commit access. davidfromonline@gmail.com, please

Do you need me to commit on your behalf? If so, which email address would you like me to use for attribution?

In D75813#2640789, @AlexanderLanin wrote:

It's been a while and I almost don't remember what this is. But it was a good fix and there is one LGTM.
Can someone please commit this? Alexander Lanin <alex@lanin.de>
Thanks!

LGTM, thanks!

LGTM, thanks!

I'd really like to hear from @delesley about these changes, specifically because of this bit:

LGTM, thanks!

LGTM, thank you!

I think this basically LGTM with a few minor nits, but I'd like to make sure @rsmith doesn't have concerns, so please wait a few days before landing in case he wants to chime in.

LGTM!

LGTM!

LGTM!

Thanks! I've commit on your behalf in 5a87f81fe9aee996dfe3a84dd833f0a48e093e7f

In D97371#2641800, @rjmccall wrote:

Windows exceptions code-generation is quite different; I don't know whether Clang supports ObjC on Windows in general. It'd be fine if you add a -triple argument to this test.

In D55212#2641402, @erik.pilkington wrote:

In D55212#2641301, @aaron.ballman wrote:

I think folks will have a chance to react if we apply it to trunk now, but I am wondering if anyone's had a chance to investigate usage in the wild. @erik.pilkington or @dexonsmith, do either of you have a gut feeling?

I don't see any uses of #pca with alloc_size in our internal codebase. IMO I'd be really quite surprised if anyone actually used these two features together.

In D55212#2640260, @arichardson wrote:

I completely forgot about this patch. @aaron.ballman are you still happy for me to commit this?

LGTM, thank you!