riccibruno 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. Sort of... I went through the list of bit-fields classes and if I count correctly there is 4 (possibly 5) bits left in `Stmt` or `Expr` which can be used without doing anything else. I guess that using one of them is okay. > 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
