Thanks, will you need someone to commit this for you?

I'd like to also see a testcase for the situation where we trigger the emission of a declaration with an available_externally definition and then find we need to promote it to a "full" definition:

Oh, and you'll need an accompanying test case before this can be committed (look at the existing tests in test/SemaCXX for ideas).

Thanks!

In D35678#840092, @boris wrote:

My understanding of your comment is that the rest is ok for now (since there will probably be a redesign in this area). If, however, I misunderstood and you sill want to move the id flattening to the caller, let me know.

In D36338#832543, @davidxl wrote:

This looks fine to me. Is there any performance data with PGO?

This looks reasonable to me. Please consider whether we should take D36604 too.

In D35941#823524, @alexfh wrote:

IIUC, most cases where -Wshadow warnings are issued is when a declaration from an enclosing scope would be accessible if there was no declaration that shadows it. In this case the the local variable of a function would not be accessible inside the local class anyway

Committed as r310401.

In D35056#834705, @rnk wrote:

In D35056#834689, @rsmith wrote:

I also removed some incorrect assumptions from the Win64 ABI code; this changed the behavior of one testcase from uncopyable-args.cpp (implicitly_deleted_copy_ctor::A is now passed indirect).

That's probably not correct, let me take a look... I remember breaking the rules for small types here.

Remove added calls to DeclareImplicit* and ShouldDeleteSpecialMember. In their place, figure out whether an implicit special member would be deleted or not by querying the AST.

As requested by Vassil, I'm going to upload another version of this that avoids declaring implicit special members so frequently.

The current behaviour is correct according to the latest discussions of CWG. We shouldn't be delaying instantiation; we just need to fix the recent regression for functions where we try to instantiate functions that don't have a complete body yet.

In D36338#832522, @iteratee wrote:

Should we adjust the ratio if it's not specified explicitly and no profile data is available?

FWIW, I see a 10% performance improvement on one of our internal benchmarks with this patch + force-precise-rotation-cost, compared to force-precise-rotation-cost alone.

This will need a test case.

Landed in r309975.

Sorry for the delay. Let's get this in for Clang 5.0. Thank you!

Organizationally, this seems fine. Carry on :)

LGTM, thanks!

LGTM with a tweaked comment. Thanks!

I would prefer to handle std::byte explicitly in the places where it matters, rather than adding an attribute to it and otherwise treating it like any other enumeration. We're going to want to be able to identify std::byte for warning emission and probably other codegen purposes too -- it's more a new fundamental type that happens to be defined in the library (like std::nullptr_t) than a type that just happens to be mayalias. Maybe a Type::is[Std]Byte, analogous to Type::isAlignValT?

I think we need more consideration of what the end result of __VA_OPT__ should be, and particularly how it should affect the SourceLocation data. It seems there are two models:

It looks like there are fewer special cases with this direction then our present one, though I worry that they'll be less obvious. On the whole, this seems like a improvement.

Looks like most of the users of this function are detecting whether we're in the builtins buffer. Perhaps we should have a dedicated method for that which uses something more principled than a string comparison against "<built-in>"?

Sure, it makes sense for UBSan to behave like other sanitizers in this regard.

This needs tests; perhaps extend test/Driver/linux-header-search.cpp for this.

We need to be completely clear on the differences and interactions between _Float16 and __fp16; you should include a documentation update that describes this as part of this change or as a companion change. In particular, an explanation of why we need two different types here is essential.

Some concrete suggestions throughout the patch, but I think we should take a step back and reconsider this warning approach: it seems bizarre for us to warn on any packed struct that happens to contain a char. It would make sense to warn if an __attribute__((packed)) written in the source has *no* effect (because it's applied to a struct where all fields already have alignment 1, or because it's applied to a field that has alignment 1) -- even then I'm not entirely convinced this is a valuable warning, but I assume there's some reason you want to warn on it :)

We've had problems in the past with speculative emission of values like this resulting in us producing invalid symbol references. (Two cases I remember: an available_externally symbol references a symbol that should be emitted as linkonce_odr but which is not emitted in this TU, and an available_externally symbol references a symbol with hidden visibility that is actually defined in a different DSO. In both cases, if the value of the available_externally symbol is actually used, you end up with a program with an invalid symbol reference.)

I don't especially like that we pretty-print the substituted subexpression to name the requirement, but I'm also not sure any other alternative would be better.

I'm not particularly happy with the number of different places where we duplicate the knowledge of how break/continue bind to enclosing loops, but fixing that seems beyond the scope of this change.

Thanks, do you need someone to commit this for you?

You should remove the code after the assert(0), or you'll trigger "unreachable code" warnings.

LGTM, post-commit review is fine for changes like this.

In D34909#797252, @kastiglione wrote:

There are other drivers with the same issue. If there's no opposition to this, I can update those here as well.