Thanks, this looks like a good broad direction.

LGTM

In D59038#1422905, @hans wrote:

In D59038#1422890, @rsmith wrote:

LGTM, and I think this is safe enough to take for Clang 8.

Do you think the severity is high enough to spin another release candidate?

LGTM, and I think this is safe enough to take for Clang 8.

In principle, I think an extension in this space seems reasonable and useful. Considering the points in http://clang.llvm.org/get_involved.html, I'm happy to assume you meet points 2, 5, and 6, but I'd like to see a written-up rationale for the other points. (In particular, you do not yet meet the bar for points 3 and 7, and if you're not proposing this to WG14 or WG21 or similar, some amount of rationale justifying that is necessary.)

In D58154#1420565, @aaron.ballman wrote:

If we go with a different name for the flag, then the user has to update their build scripts to get code to compile with Clang, which means it shouldn't be too onerous for them to spell out the specific diagnostics they need disabled (and it sort of forces them into somewhat better code hygiene by not disabling all diagnostics). I'm kind of leaning towards not providing a flag at all.

In D58154#1419956, @jyknight wrote:

I also can't really tell what the intent is behind classifying some diagnostics in Clang as ExtWarn,DefaultError and others Warning,DefaultError -- or if there even is any useful purpose there at all. I note with special-puzzlement the existence of both ext_return_missing_expr and warn_return_missing_expr.

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>.

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?

This seems fine to me, but I think we can do better if we want to. How about: call TryCreateTempFile() N times, and check that it succeeds exactly 16 times. Pick N such that the probability of it failing to find all 16 possible file names (given that it has 128 retries for each call) is astronomically small. (Right now the test should fail somewhere around 0.03% of the time; if we made, say, 32 calls, the probability of a flake would be significantly lower than the probability of a flake due to spontaneous hardware failure (in order to flake, we must find only 15 of the 16 filenames with at least 17 * 128 + 15 guesses, the probability of which is 1 in 3.9x10^62).

In D58149#1397499, @ldionne wrote:

In D58149#1397390, @jfb wrote:

In D58149#1397382, @efriedma wrote:

Formally, I don't think C11 is a normative reference for C++11 or C++14, only C++17 (see [intro.refs] in the standard).

You're right. I don't mind submitting a patch that enables it for C++17 and above only if that's what you want, however...

Right, this was changed in wg21.link/p0063r3
That being said, we *can* offer these C11 features as extensions IIUC.

... I think it's also fine to have it in C++11.

Address review comments.

Combine WithinStmtExpr flag with AllowedStmtKinds into a more general statement
context. In doing so, fix some bugs where the OpenMP context was not being
propagated properly through labels and expression-statements starting with @.

In D57984#1394067, @ABataev wrote:

In D57984#1394050, @rsmith wrote:

@ABataev Is it intentional that we do not propagate Allowed through labels? For example:

void f() {
  #pragma omp barrier // ok

label:
  #pragma omp barrier // error, "cannot be an immediate substatement"

label:
  ;
  #pragma omp barrier // ok
}

?

No, it is a bug.

@ABataev Is it intentional that we do not propagate Allowed through labels? For example:

LGTM

Address a couple of review comments.

Thanks, this is a much cleaner model than having two different APValue representations for arrays.

You'll also need to update TreeTransform::TransformCompoundStmt. (And please add some tests for template instantiation.)

The "false negatives" in the current behaviour are the result of an intentional decision to avoid false positives for unsequenced operations that cannot actually both happen as part of the same evaluation (because both are conditional on different and mutually-exclusive conditions), such as the false positive we now get in the tests. I think it's reasonable to revisit that decision, though; the new false positive cases do not realistically seem likely to occur in real code whereas the false negatives do.

In D57626#1382391, @Quuxplusone wrote:

I admit that this lock_guard example is contrived and generally ill-advised, but its ill-advisedness seems like a higher-level concern that shouldn't be "enforced" by fiddling with the rules of [[trivial_abi]], so I hope that's not what's going on here.

Thanks, LGTM.

In D56760#1368279, @erik.pilkington wrote:

FWIW I'd prefer the __builtin_object_size spelling too, but it doesn't seem like the GCC folks are super crazy about it to me. So it seems likely to me that if we implement it it will just be a clang extension for at least the medium term (if not permanently). I guess that's fine, so long as the GCC people are aware that it would be bad to extend their builtin to use type&4.

LGTM, thanks.

Looks good to me, thanks! Let's try this and see if it shakes out any problems.

There isn't really a standard name for this; in the absence of such a name, this seems as good as anything.

Thanks for the added tests! They seem to have found a bug in the handling of local const integral variables.

In D56760#1368054, @erik.pilkington wrote:

So it seems like the GCC people want to keep __builtin_object_size static.

In D56723#1366529, @ilya-biryukov wrote:

I'll try playing around with your idea, my initial plan is to store preferred type alongside the current token as a member of the Parser class and update it when advancing to next token, when the parser backtracks and in the places where we care about propagating the types.

This seems to be introducing a requirement that enterUnknown is called on all paths through the parser where we recurse to parse a subexpression and don't have specific type information. That seems like an unfortunate requirement to me from a maintenance perspective; have you considered alternative approaches? For example, we could store the preferred type alongside the SourceLocation of the corresponding token (and propagate the information when we parse parentheses and any other completion-type-preserving construct), and only apply the information when the code completion token is at that location. That way, we only need to annotate cases where we know the preferred type, not when we don't.

Thanks, looks great!