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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D65591#1625228 <https://reviews.llvm.org/D65591#1625228>, @ilya-biryukov wrote:

> 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 have 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.


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.

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? Does anything special need to happen for modules?
- I would expect to see this information reflected in an AST dump
- How should this impact AST matching interfaces?
- Test cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65591