LGTM aside from a NFC change

LGTM aside from a small docs nit (be sure to regenerate the docs after you fix it).

You should also add Sema tests that ensure the attribute applies to the expected AST nodes, is diagnosed appropriately when applied to something it shouldn't be applied to, accepts no args, etc. Basically, all of the semantic places we could warn on should have test coverage, as well as demonstrations of correct usage.

I worry a little bit about our -Wtype-limits getting out of sync from GCC's due to making it a synonym for -Wtautological-constant-in-range-compare, but I'm also at a loss for why these two should ever have different functionality.

Adding Delesley in case he has input on design.

LGTM!

In D59520#1435728, @sunfish wrote:

In D59520#1434854, @aaron.ballman wrote:

Removes errnoneous use of diag::err_alias_is_definition, which turned out to be ineffective anyway since functions can be defined later in the translation unit and avoid detection.

If you need to keep the prohibition that these attributes not be applied to functions with definitions, there are ways to accomplish that (we merge declarations and can decide what to do at that point if we see a declaration that is a definition). Given that, do you still want to lift this restriction?

These attributes don't make sense on definitions. So right now, they are silently ignored when applied to definitions. That's effectively the current behavior too, because the existing check doesn't work as intended, so the change here doesn't change anything in practice yet.

A diagnostic would be slightly tidier, but I'm not familiar enough with clang to do this quickly and didn't want to delay the rest of the patch while I investigated. Do you know the specific places we'd need to change to diagnose this?

This seems reasonable to me.

LGTM!

Would you mind committing the changes please? Thanks.

I love functional changes that remove code and add tests -- thank you for these!

LGTM aside from a small nit.

In D59467#1432675, @Tyker wrote:

if likely/unlikely can be applied to any statement or label what should be generated when it is applied to a statement that don't have any conditional branch ? should it be ignored ?

While I'm okay with this, I would mildly prefer it if landed with at least one check rather than entirely empty (IIRC, that's how we'd added new modules in the past).

LGTM aside from some small nits. Feel free to fix and commit without further review if you'd like.

I believe this has the incorrect modeling -- in the following code example, the attribute appertains to the substatement of the if statement, not the if statement itself:

if (foo) [[likely]] {
  blah;
}

This falls out from: http://eel.is/c++draft/stmt.stmt#1. You model this by applying the attribute to the if statement, which will have the wrong semantics when you hook up the hints to the backend.

LGTM aside from a nit.

LGTM aside from some minor nits.

LGTM!

I think the release notes should be updated to mention that a previously-supported option has been removed and that there's a new option available as a replacement; I don't think we have any other docs for pp-trace to worry about updating, do we?

In D58896#1428816, @edward-jones wrote:

In D58896#1419964, @aaron.ballman wrote:

Do you have some evidence that the current behavior is causing a lot of false positives in the wild? For ASCII character literals, I can sort of guess at why people might want to do this, but for things like wide character literals, or character literals relying on the current code page, etc, I'm less convinced.

I don't know about the false positive rate, just the one report on twitter which triggered me to submit this change. As for wide character literals I was under the impression that they would be promoted to integers and wouldn't have triggered the -Wchar-subscript anyway?

This is missing test cases that demonstrate the behavior is what we expect it to be for ObjC++ code vs C++ code.

LGTM!

This is valid C++ codes. Both GCC and MSVC accepts this.

I feel like I must be missing something, because MSVC does not accept the code from your test case: https://godbolt.org/z/r6evxY Are there other language options required to get MSVC to accept this code, or perhaps is it only accepted by certain versions of MSVC?

In D59103#1422775, @kallehuttunen wrote:

Another idea that came to my mind would be to enable this check only for annotated types. So warning for missing field access would be only given for types that have for example [[clang::annotate("value type")]] annotation. Possibly other kinds of checks could be also developed for types that have the given annotation.

I think we're pretty close! Some of the testing code can be cleaned up, but I could also use some help verifying that we're correctly matching the behavior of GCC as well.

In D59135#1422781, @thorsten-klein wrote:

Hello,
Can you please support how to do that? :-)

Can you add test coverage that demonstrates the fix behaves as expected?

I'm definitely worried about the false positive rate for this check -- from a quick look at the diagnostics from LLVM, every one of them is a false positive (though perhaps I've missed something; it was a very quick look). Requiring users to explicitly use a declaration within the operator seems like it could get onerous for complex classes that support the notion of equality from a few members as opposed to all members.

In D58154#1420344, @rsmith wrote:

I don't have particularly strong feelings one way or the other about accepting -fpermissive at all. For GCC compatibility, it seems like a moderately useful thing to support, but I don't think we have any interest in accepting everything that GCC accepts under -fpermissive.

In D58216#1420149, @rafauler wrote:

Both approaches make sense to me. I'll re-land the previous patch in favor of gcc compatibility, since the semantics of attribute used in member functions of class templates were first implemented in gcc.

Do you have some evidence that the current behavior is causing a lot of false positives in the wild? For ASCII character literals, I can sort of guess at why people might want to do this, but for things like wide character literals, or character literals relying on the current code page, etc, I'm less convinced.

LGTM aside from some minor nits, but you should wait for someone more well-versed in the static analyzer to sign off before committing.

In D58216#1414864, @modocache wrote:

Friendly ping! @aaron.ballman it looks like you accepted D56928, could you take a look at this as well?

In D52835#1412416, @xbolva00 wrote:

@aaron.ballman does it make sense to warn for this case only in C/pre-C++11 mode? Since this case in C++11/14/17 is handled by -Wnarrowing, as we see from tests.

In D58154#1418594, @jyknight wrote:

The errors disabled by this feature are default-error warnings -- you can already get the same effect by using -Wno-<lots-of-things>. Why is it bad to additionally allow -fpermissive to disable them? (If we have any diagnostics which are currently default-on-warnings which should not _ever_ be disable-able, then maybe we should just fix those?)

In D58154#1417474, @rsmith wrote:

In D58154#1417363, @aaron.ballman wrote:

I'm not super keen on supporting -fpermissive -- what is the impetus for supporting this? Is it just for GCC compatibility?

Yes, exactly that -- this is to improve our drop-in compatibility with GCC. See http://llvm.org/PR40670 for an example user request for this.

I'm not super keen on supporting -fpermissive -- what is the impetus for supporting this? Is it just for GCC compatibility?

LGTM!

LGTM with a minor documentation nit.

Still LGTM, thanks for adding the license snippet!

@aaron.ballman, are you actually interested in reviewing these attributes? 'Cause George added you on his reviews but i totally understand that these are fairly exotic.

Please be sure to update Registry.cpp to expose the new AST matcher to the dynamic matcher infrastructure, regenerate the AST matcher documentation (run clang\docs\tools\dump_ast_matchers.py), and add tests for the matcher.

In D57948#1399403, @riccibruno wrote:

(Following up on a discussion on IRC) I have looked at what would be needed to revert the set of patches which introduced this change, but this results in a >1k lines diff which do not apply cleanly. I think it might be safer to just merge this fix. Unless @aaron.ballman disagree, @hans could you please merge this into the release branch ?

LGTM!

LGTM!

LGTM with a testing request.

This seems sensible to me, but I leave it to the libc++ maintainers to sign off on the review. Thank you for working on this!

In D58091#1396397, @jdoerfert wrote:

In D58091#1396382, @aaron.ballman wrote:

• but I wonder why those diagnostics are happening in the first place. It seems like the warning is still useful when it triggers outside of that situation, no?

The underlying conceptual problem, which I didn't know when I added GE_Missing_type, is that this has _nothing_ to do with the location of the declaration. We say, include the header X.h, if we were not able to build a type for recognized built-in Y that should be declared in X.h. However, we should report _why_ we could not build the type instead. For built-ins we do not have a type on record (GE_Missing_type), this is always, so no warning for now. For the ones that we only fail to build a type because some requirement is missing, we should report that, at least when we are in the respective header. I don't have a perfect solution of what to do actually.

I could check if the declaration is (probably) in the respective header so we can switch between warnings?

If I'm following along properly, it sounds like we want to disable this warning largely because it can appear in header files attempting to declare the functions in question -- but I wonder why those diagnostics are happening in the first place. It seems like the warning is still useful when it triggers outside of that situation, no?

LGTM!

LGTM!

LGTM!

LGTM!

Oops, sorry, I think I added some comments to a previous revision of the review. To recap:

Considering that this has been fertile ground for buggy edge cases, should we revert my original changes from the 8.0 branch to give this more time to bake on trunk before releasing? Or do you think this is fine to move onto the release branch once finalized?

LGTM aside from some very minor nits.