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

[Haojian Wu via Phabricator via cfe-commits]

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