hokein added a comment. In D65591#1927076 <https://reviews.llvm.org/D65591#1927076>, @sammccall wrote:
> This is nice and simple, I do wonder whether we're missing cases in > ComputeDependence.cpp for the new bit. > > Can you take a pass through to check? (Assuming you haven't since picking up > this patch from Ilya). I'll do the same. will do it. unfortunately, we are missing tests in this patch -- most AST nodes are dropped when clang sees semantic errors, so it is hard to write tests. after https://reviews.llvm.org/D69330, it'd be better, I think we can add them afterwards. ================ Comment at: clang/include/clang/AST/ASTDumperUtils.h:65 static const TerminalColor ObjectKindColor = {llvm::raw_ostream::CYAN, false}; +// contains-errors +static const TerminalColor ErrorsColor = {llvm::raw_ostream::RED, true}; ---------------- sammccall wrote: > I wonder if coloring the whole subtree red is an overreaction, but it may not > be. Let's see :) I used it for a while, didn't see any big issue about it, we only color the `contains errors` word. ================ Comment at: clang/include/clang/AST/DependenceFlags.h:46 Instantiation = 2, + /// Placeholder, and is not used in actual Type. + Error = 4, ---------------- sammccall wrote: > I'd like this comment to explain why it exists if not used in actual types. > > Is this used for `decltype(some-error-expr)`? Is this used so > toTypeDependence() returns something meaningful? > > If this is used to make the bitcasting hacks work, we should just stop doing > that. yeah, the main purpose of it is for convenient bitcast. AFAIK, we don't have a plan to use the error bit except in `Expr`. removing it for now. 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
