In D66125#1642013, @jkorous wrote:

BTW It just occurred to me that multiplexing ASTConsumer::shouldSkipFunctionBody must have hit the same issue as FrontendAction multiplexing we considered above.

bool MultiplexConsumer::shouldSkipFunctionBody(Decl *D) {
  bool Skip = true;
  for (auto &Consumer : Consumers)
    Skip = Skip && Consumer->shouldSkipFunctionBody(D);
  return Skip;
}

LGTM

LGTM. Kill it with fire

This actually seems useful. If one starts the search at operator bool, it's nice to see its implicit calls too.
What are the cons of having this?

LGTM, thanks!

LGTM

LGTM

• Change / to

• Expose the existing helper instead of introducing a new one

LGTM from my side (and a few NIT, but up to you whether to apply them)

LGTM

LGTM

Thanks for providing examples of error messages, also agree they look fine.
If we find bad error messages later, we could revisit the presentation strategy.
LGTM

• Rename the helper function
• Remove mention of 'out-of-line definition' from a comment

Surfacing errors to the users in those cases is definitely something we need to do, thanks!
How do they look in practice? In particular, should we add more context information (either in clangd or in the VSCode extension) to those errors, now that we know they are shown to the users?
Something like Failed to apply a fix: input range is invalid vs input range is invalid.

This patch mentions there is code currently that "reconstructs the mapping". Why not remove it in this patch? Where is it used if we actually serialize the mapping?
Do we have any measurements? How faster is this in particular use-cases? How bigger

I did some experiments with adding something similar to the ErrorExpr Aaron suggest. It has this flag set and does not get removed from the AST.
I believe the best way to address Aaron's comments is to add tests mentioning this expression instead of trying to catch TypoExpr, which normally get removed from the AST.

In D65591#1625744, @aaron.ballman wrote:

The problem is: those bits are not infinite and we only have a handful left until bumping the allocation size; is this use case critical enough to use one of those bits? I don't think it will be -- it seems like premature optimization. Also, by calculating rather than using a bit, you don't have to touch every Expr constructor, which reduces the complexity of the patch.

Alternatively, we could keep an equivalent of set<Expr*> InvalidExprs in ASTContext. Gives the same computational complexity, but has a higher constant overhead factor.
Does that look reasonable?

The heuristics for properly rendering the comments are probably a good way to go.
I'd say this change is still a step in the right direction - text blocks in formatted strings should be properly escaped to avoid being interpreted like markdown constructs.
Any structure we want to have should be explicit.

In D65591#1625183, @aaron.ballman wrote:

In D65591#1625154, @riccibruno wrote:

It seems that these two options are not exactly the same right ? The ContainsError bit is useful to quickly answer "Does this expression contains an invalid sub-expression" without doing the search, while adding an ErrorExpr node is useful to note that this sub-expression is invalid (and as Aaron says the hypothetical ErrorExpr node can carry more info about the error).

That's true. I had figured that answering "does this expression contain an invalid sub-expression" could be implemented with a walk of the expression tree rather than consuming a bit. To consumers of containsErrors(), there shouldn't be much difference aside from performance (and that may be sufficient reason to go with a bit, but I think I'd like to see performance measurements that show the bit is necessary).

Are expression bits scarce?
We don't any checks if expressions contain errors now, we simply drop too many invalid expressions and never put them into the AST.
It's impossible to do the measurements at this point, but it would be nice if adding those checks was cheap.

In D65591#1625154, @riccibruno wrote:

It seems that these two options are not exactly the same right ? The ContainsError bit is useful to quickly answer "Does this expression contains an invalid sub-expression" without doing the search, while adding an ErrorExpr node is useful to note that this sub-expression is invalid (and as Aaron says the hypothetical ErrorExpr node can carry more info about the error).

Exactly right, thanks for summing this up!

LGTM. Great example for the test case! It will definitely stay valid even if we fix all problems caused by RecursiveASTVisitor!

Gentle ping. @rsmith, please take a look when you have time.

In D65752#1623914, @sammccall wrote:

This looks reasonable to me, there are a couple of variations you might think about:

• also treat QualifiedNameLookup as an option, and override in places with an RAII pattern like ScopedOverride Unqual(QualifiedNameLookup, false) (why don't we have a class like that?). This would further reduce args.

I actually think it's good to have it as an explicit option. Qualified and unqualified lookup are so vastly different use-cases, that having the flag that discriminates them as a function parameter looks like a better trade-off.
Besides, RAII objects are more code and more complicated.

LGTM, thanks!
We should also merge this into the release branch if it's not too late yet.

LGTM, but please print the function template arguments uniformly before landing this (to avoid different forms of outputs inside the same test).

One important comment about somehow distinguishing multiple decls with the same name.

• Add the missing newline

• Make sure the test actually runs IncludeFixer.cpp

This is just a proposal, there are probably other ways to reach better readability, e.g. group some of those parameters into a struct.
But let me know what you think, happy to refactor in a slightly different manner or simply drop this revision.

• Remove accidental change from revision

LGTM from my side, a few optional NITs.
Feel free to land as soon as @hokein stamps.

The fix looks good.

Thanks!

@hans, could we merge this commit into the release branch?

LGTM

Could this be handled outside clangd? If not, what are the complications?

Neat, thanks for the cleanup! A few suggestions from me, no major comments.

SG, just wanted to make sure it's on your list ;-)

@hans, could you please merge rL367530 into the release?

• Update a comment

LGTM from my side

• Reformat

• Add a test

An experiment with popping on expression evaluation context failed, I couldn't find a good way to fix the problems described above.
Typo correction can and will run after the evaluation context where expression created is popped and the diagnostic we produce depends on the results of typo correction.
Emitting diagnostics on some, but not all context pops could be an option, but classifying those seems to be hard, would require looking closely at every expr eval context push.