Interesting. I think all of the new warnings in the test cases here are undesirable (they duplicate errors produced by the constant evaluator), but the removed warnings all look like improvements.

In D62009#1506415, @Tyker wrote:

there are issues with using ConstantExpr to mark expressions as constant for semantic checking:

• #1 multpile Expr::Ignore* operation remove ConstantExpr from the expression.

In D61722#1504376, @sammccall wrote:

In D61722#1503863, @rsmith wrote:

I expect we'll want a ContainsErrors bit on Stmt, propagated similarly to the existing InstantiationDependent / ContainsUnexpandedParameterPack / ... bits.

This sounds good (as an alternative to using dependent bits for this).

Do you think we want a similar bit on Type (set on e.g. vector<ErrorTy>) , or should be ensure ErrorTy never gets used for compound types?

This implementation is accepted by the lowest supported versions of all of our supported compilers: https://godbolt.org/z/yZJDu3

In D62009#1505263, @Tyker wrote:

i added this bit because when we eventually store the result of evaluation in ConstantExpr we will probably want to trail-allocate the APValue's possible representations separately to limit memory consumption. but if we do this the ConstantExpr node will need to be created after evaluation of the value to allocate the right space and we will need an other way to mark that the expression should be evaluated in constant context.

The approach we've been taking for this up until now is to use a ConstantExpr node to mark the entry point of a constant context; it looks like that would continue to work here, but we're missing those nodes in some cases? (This is preferable to using a flag because it also -- eventually -- gives us a place to store the evaluated value, which we're going to need for various upcoming features, particularly consteval.)

Thanks, LGTM!

LGTM, thank you! :)

In D58920#1503872, @modocache wrote:

Thanks for the help, @rsmith! Your suggestions were spot-on. (It took me a little while to figure out why, even using the LazyDeclPtr directly, I was still triggering deserialization. It turns out dump() causes deserialization too -- whoops!)

You should also change FindExistingResult::~FindExistingResult to update Sema::StdNamespace to point to your newly-deserialized namespace if you didn't find a prior declaration of it, so that Sema::getStdNamespace() finds the deserialized namespace.

I haven't done this yet. I'm trying to think of a test case that would fail if this were not done properly -- or would there not be one?

I expect we'll want a ContainsErrors bit on Stmt, propagated similarly to the existing InstantiationDependent / ContainsUnexpandedParameterPack / ... bits. For example, the constant evaluator will want to quickly bail out of evaluating expressions and statements containing errors, and the error recovery TreeTransform that we perform to fix typos will want that too (and maybe could be able to fix other kinds of errors for which we build these error nodes eventually?).

Generally, I think this is a good, useful feature, but there's one point from http://clang.llvm.org/get_involved.html 's checklist for accepting language extensions on which its case is weak, as reflected by this feedback from @alexr on D17741:

I don't think we want to add a fundamental new preprocessor feature like __FILE_BASENAME__ without at least getting some early buy-in from the WG14 and WG21 committees.

In D36357#1501999, @Rakete1111 wrote:

In D36357#1501961, @rsmith wrote:

In D36357#1500949, @Rakete1111 wrote:

How should I do this? Do I just skip to the next }, or also take into account any additional scopes? Also does this mean that I skip and then revert, because that seems pretty expensive?

It would be a little expensive, yes, but we'd only be doing it on codepaths where we're producing an error -- for an ill-formed program, it's OK to take more time in order to produce a better diagnostic. Skipping to the next } won't work, because SkipUntil will skip over pairs of brace-balanced tokens (so you'll skip past the } you're looking for), but skipping until the next { and then skipping to the } after it should work.

Hmm wouldn't this interact badly with {} in initializers?

[]<int = {0}> {};
[](int = {0}) {};

In D36357#1500949, @Rakete1111 wrote:

How should I do this? Do I just skip to the next }, or also take into account any additional scopes? Also does this mean that I skip and then revert, because that seems pretty expensive?

In D59885#1501774, @ilya-biryukov wrote:

The suggested approach looks promising. The difference seems to be within the noise levels on my machine:

Thanks for working on this!

The problem may well be that you're recursively triggering deserialization of the std namespace from its own deserialization.

Thinking about this some more: distinguishing between "macro expansion" and other cases seems like a proxy for "this token came from inside the preprocessor / lexer" versus "this token was provided by the user", which is also exactly what IsNewToken is supposed to capture. I don't think we need two separate flags here.

Yeah, this seems to match the library wording. (I think it's short-sighted and over-fitting -- this is baking into the library specification that the language happens to not allow unions to have base classes today -- but this isn't the venue for that discussion.)

In rL360464#648749, @RKSimon wrote:

In rL360464#648724, @RKSimon wrote:

@rsmith This is failing on MSVC boxes, please can you take a look: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/25723

Sorry @rsmith due to dependencies, I had to revert rL360499 and rL360464 together in the end

If I understand correctly, you want to be able to compile a program against some struct and union layouts, and then at load time "update" the program to cope with the actual layouts for those types being something else, but containing (at least) the set of members you know about. And your proposed approach is to add intrinsics into the IR that identify the GEPs that we emitted to model struct field accesses, but to otherwise not change the emitted IR. If so, I don't see how this approach to that problem can work. There seem to be a couple of problems:

In D59885#1496734, @ilya-biryukov wrote:

Overall, the performance cost in empty-callback case seems to be lower than 5%. This is significant, but hopefully acceptable.

Remove unneeded test change.

If you're happy with these two conditions, then I have no concerns with this moving forward: