[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D65591#1625744 <https://reviews.llvm.org/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?

> Some other things I think are missing from the patch (regardless of whether you go with a bit or calculate on the fly):
> 
> - Do you need some changes to AST serialization and deserialization?

Good point, will update the patch.

> - Does anything special need to happen for modules?

Not sure. What are the potential problems you foresee?

> - I would expect to see this information reflected in an AST dump

Good point. Will do. Although it's a little hard to test in this patch, since it's hard to catch a `TypoExpr` in the AST dump.

> - How should this impact AST matching interfaces?

We could add a matcher that filters on this flag, but I would start with adding more expressions first (something similar to `ErrorExpr`);
For the purposes of this patch, I'd keep the matcher interfaces untouched.

> - Test cases

Again, since it's hard to catch a `TypoExpr` in the final AST dump, it's hard to catch this bit. See the dependent revision for a bogus diagnostic not being emitted anymore.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65591/new/

https://reviews.llvm.org/D65591