LGTM!

LGTM!

LGTM aside from a minor nit.

LGTM as well

Aside from an assert than can be removed, this LGTM on the attribute side of things.

Adding Richard to see if he agrees with the direction taken.

The attribute bits look good to me, but I am less familiar with the behavioral bits so you may want to wait to see if @majnemer or @rnk have further comments on that.

LGTM aside from a style nit (you can take it or leave it).

LGTM with a formatting nit.

In D49275#1162204, @aaronpuchert wrote:

Thanks for the review. Could you commit this for me (Aaron Puchert <aaron.puchert@sap.com>)?

LGTM, though it's your call on the const_cast stuff whether you want to revert or keep it.

LGTM! Thank you for this!

I believe this would be non-comforming.

LGTM! Thank you for doing this work, Erich!

In D48912#1156191, @ldionne wrote:

Regarding whether we want to enable or disable those warnings by default: it seems like we've reached consensus (on the LLVM IRC) for enabling the deprecation warnings by default. However, I suggest we keep them disabled by default in this commit, and then change the default in a subsequent change. This will make things easier from an integration perspective, and also in case we need to roll back the defaults because of some unforeseen circumstances.

LGTM! Do you need me to commit on your behalf, or do you have commit privileges?

LGTM, thank you!

In D48852#1153598, @arphaman wrote:

In D48852#1153415, @aaron.ballman wrote:

This is acceptable because Darwin guarantees that, despite the watchOS ABI differences, sizeof(ptrdiff_t) == sizeof(NS[U]Integer)

Can you describe these ABI differences please? Also, does Darwin guarantee that alignof(ptrdiff_t) == alignof(NS[U]Integer)?

The ABI difference boils down to the following:

Regular 32-bit Darwin targets (like armv7) use 'ptrdiff_t' of type 'int', which matches 'NSInteger'.
WatchOS arm iOS target (armv7k) uses 'ptrdiff_t' of type 'long', which doesn't match 'NSInteger' of type 'int'.

This is acceptable because Darwin guarantees that, despite the watchOS ABI differences, sizeof(ptrdiff_t) == sizeof(NS[U]Integer)

In D44143#1152550, @boga95 wrote:

How can I commit it?

In D48912#1152573, @ldionne wrote:

FWIW, I would prefer to enable deprecation warnings by default and to have a way of turning them off

This mostly looks good, but the name of the matcher is inconsistent.

Aside from a new round of minor doc nits, I think this is looking good.

LGTM!

This still LGTM; do you need someone to commit on your behalf?

Wow, this is an amazing start to a very large refactoring! Thank you for tackling this. I've made a few review comments but haven't gotten through the entire patch yet. The parts that I've seen all look reasonably sane thus far.

In D48717#1147644, @JonasToth wrote:

• fix decltype deduction with new matcher
  
  I had to introduce a new matcher in clang for DecltypeType to reach its underlying type. It would be better to have hasType resolve all these issues but i dont know how much work that would be.

Once this goes in, you can also update TrailingReturnTypeCheck.cpp to remove its local instance of this type matcher.

LGTM with a small documentation nit.

LGTM!

In D46313#1138098, @homerdin wrote:

Sorry about the slow turn around. I've updated the documentation to be clearer about this attribute's use.

While I'm mainly interested in marking math functions, exposing this attribute to users should be beneficial as I'm working to enable some optimizations in the backend making use of the WriteOnly LLVM attribute.

I haven't added any additional documentation concerning consequences and effects because at this point the WriteOnly LLVM attribute is not being used extensively. Would it be best to wait until additional uses (CSE, Hoisting) are in effect then update the documentation?

LGTM aside from some documentation nits.

LGTM!

Do you also want to update AttrDocs.td with some similar information?

In D47953#1137375, @paulsemel wrote:

I will for sure add tests @lebedev.ri . Fact is that, for the moment, this is not working as expected.
This is why I am asking for a bit of help about this bitfield handling :)

LGTM, thank you for this!

I've committed in r335048; if @rsmith has concerns, they can be addressed post commit.

LGTM aside from some minor nits.

Thank you for working on this odd detail of attributes!

I had to revert due to failing tests. The revert was done in r334606 and this is an example of a failing bot: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/31500

I've commit in r334604 with one minor change -- I added an include for <cctype> to the unit test so that std::isspace() would compile properly on all platforms.

Updating based on review feedback.

Ping

In D47290#1127190, @jfb wrote:

In D47290#1126443, @aaron.ballman wrote:

In D47290#1125028, @aaron.ballman wrote:

Okay, that's fair, but the vendor-specific type for my Windows example is spelled DWORD. I'm really worried that this special case will become a precedent and we'll wind up with -Wformat being relaxed for everything based on the same rationale. If that's how the community wants -Wformat to work, cool, but I'd like to know if we're intending to change (what I see as) the design of this warning.

I spoke with @jfb offline and am less concerned about this patch now. He's welcome to correct me if I misrepresent anything, but the precedent this sets is that a platform "owner" (someone with authority, not Joe Q Random-User) can relax -Wformat as in this patch, but this is not a precedent for a blanket change to -Wformat just because the UB happens to work and we "know" it.

Thanks for asking these questions Aaron, it's helped answer everyone's concerns and explain our respective positions. You've certainly summarized what I was thinking.

In D47290#1125028, @aaron.ballman wrote:

Okay, that's fair, but the vendor-specific type for my Windows example is spelled DWORD. I'm really worried that this special case will become a precedent and we'll wind up with -Wformat being relaxed for everything based on the same rationale. If that's how the community wants -Wformat to work, cool, but I'd like to know if we're intending to change (what I see as) the design of this warning.

In D47290#1124991, @hans wrote:

In D47290#1124964, @aaron.ballman wrote:

In D47290#1124956, @hans wrote:

In D47290#1124933, @aaron.ballman wrote:

In D47290#1124866, @hans wrote:

If we really want to special-case NSInteger, and given that you're targeting a specific wide-spread pattern maybe that's the right thing to do, I think we should make -Wformat accept (move the warning behind -Wformat-pedantic I suppose) printing NSInteger with *any* integral type of the right size, not just size_t.

Would you be similarly okay with %ld and %d on Windows platforms when mixing up int and long?

No, I'm against a general relaxation of -Wformat, but to solve JF's problem I think special-casing NSInteger might be reasonable.

How is JF's problem different?

It concerns a vendor-specific type. Of course I personally think it would be better if the code could be fixed, but it doesn't sound like that's an option so then I think special-casing for NSInteger is an acceptable solution.

In D47290#1124956, @hans wrote:

In D47290#1124933, @aaron.ballman wrote:

In D47290#1124866, @hans wrote:

If we really want to special-case NSInteger, and given that you're targeting a specific wide-spread pattern maybe that's the right thing to do, I think we should make -Wformat accept (move the warning behind -Wformat-pedantic I suppose) printing NSInteger with *any* integral type of the right size, not just size_t.

Would you be similarly okay with %ld and %d on Windows platforms when mixing up int and long?

No, I'm against a general relaxation of -Wformat, but to solve JF's problem I think special-casing NSInteger might be reasonable.

In D47290#1124866, @hans wrote:

If we really want to special-case NSInteger, and given that you're targeting a specific wide-spread pattern maybe that's the right thing to do, I think we should make -Wformat accept (move the warning behind -Wformat-pedantic I suppose) printing NSInteger with *any* integral type of the right size, not just size_t.

In D47290#1115352, @jfb wrote:

Hopefully this makes sense? I'm not sure I summarize my context very well. I'm happy to talk about it in-person next week too.

LGTM as well, for what little it's worth. :-)

LGTM!

LGTM aside from a nit with one of the tests. Once you've updated the patch and verified that check-clang passes all tests, I can commit for you next week when I'm back from meetings (unless someone else gets to it before me).

Aside from a minor commenting nit, this LGTM.

In D47201#1119947, @Hahnfeld wrote:

In D47201#1119254, @aaron.ballman wrote:

In D47201#1119249, @tra wrote:

IIUIC, nv_weak is a synonym for weak (<rant>why, oh why did they need it?</rant>)
You may need to hunt down and change few other places that deal with the weak attribute.
E.g.: https://github.com/llvm-project/llvm-project-20170507/blob/master/clang/lib/AST/Decl.cpp#L4267
https://github.com/llvm-project/llvm-project-20170507/blob/master/clang/lib/CodeGen/ItaniumCXXABI.cpp#L3045

If it is truly a synonym for weak, then a better implementation would be to make no semantic distinction between the two attributes -- just add new spellings to weak. If you need to make minor distinctions between the spellings, you can do it using accessors on the attribute.

I first went with this approach but then thought it would be better to restrict the new attribute as much as possible. That's why I added a completely new one which is only applicable to functions, but not to variables and CXXRecords. Let me know if you'd prefer nv_weak to be a full alias of weak and I'll revert to what @aaron.ballman suggested.

In D47201#1119249, @tra wrote:

IIUIC, nv_weak is a synonym for weak (<rant>why, oh why did they need it?</rant>)
You may need to hunt down and change few other places that deal with the weak attribute.
E.g.: https://github.com/llvm-project/llvm-project-20170507/blob/master/clang/lib/AST/Decl.cpp#L4267
https://github.com/llvm-project/llvm-project-20170507/blob/master/clang/lib/CodeGen/ItaniumCXXABI.cpp#L3045

LGTM

Committed in r333653.

In D45686#1115836, @dstenb wrote:

Any more comments or concerns, or can I land this?

In D47157#1115445, @bruno wrote:

Consistency would be nice, but at the same time, I don't see a good metric for when we'd know it's time to switch it to being on by default. I'm worried that it'll remain off by default forever simply because no one thinks to go turn it on (because it's silent by default). Perhaps on-by-default here and off-by-default downstream would be the better approach, or do you think this would be too disruptive to enable by default anywhere?

I believe this could be too disruptive to enable now, since it's still very common to find quoted includes in framework headers. This is very important for Modules to properly work with frameworks (quoted headers are usually considered non-modular when modules builds kick in) and is actually very compelling for us to turn it on by default on Darwin as soon as we can, but we need to educate users first.