[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#1626638 <https://reviews.llvm.org/D65591#1626638>, @ilya-biryukov wrote:

> 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?


Yup, that is also very 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'm not certain there are real problems there, but I am wondering whether something like a BMI should include serialized error AST nodes or not. Would a consumer of the module expect that? I suppose it could if we wanted it to.

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

Ah, drat, I was hoping we had at least one test that already did this, but I don't see one.

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

I think that makes sense. The sort of things I'm wondering about are: if we are going to start retaining error nodes in the AST more regularly, should AST matchers opt into matching on those nodes, or opt out of matching on them? I think the answer should be that we only AST match on valid AST nodes, but I could see arguments being made either way, so maybe this part needs an RFC for more opinions.

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

Yeah, I was hoping we'd have some AST dumping mechanism for testing this. If not, though, perhaps we could still use a unit test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65591