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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
