[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#1625183 <https://reviews.llvm.org/D65591#1625183>, @aaron.ballman wrote:

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

We can also assume they're cheap, use the visitor-based implementation and later switch if this turn out to be a problem.
I think we should prefer the approach that guarantees the compiler is not getting slow ever rather than chase the slowness later.

In D65591#1625192 <https://reviews.llvm.org/D65591#1625192>, @riccibruno wrote:

> Fair enough, especially given that IIRC there are not many bits left in these bit-fields classes.


May be reasonable in that case, but we should be aware of a potential `O(n^2)` and even exponential complexities in the frontend arising from calling `containsErrors` in too many places in that case.
If we ever run out of bits, we could remove this bit and instead introduce a cache (something like `set<Expr*> InvalidExprs`) in `ASTContext`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65591