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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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.



================
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};
----------------
I wonder if coloring the whole subtree red is an overreaction, but it may not be. Let's see :)


================
Comment at: clang/include/clang/AST/DependenceFlags.h:20
     Instantiation = 2,
-    Type = 4,
-    Value = 8,
+    Error = 4,
+    Type = 8,
----------------
Nit: it seems weird to jam this right in the middle of an enum.
Instantiation/Type/Value are about template dependence, unexpandedpack is a bit different (but still strictly C++ semantics), Error is definitely different (outside language semantics).

Maybe make error the last element?
(Making it first is weird as it's so atypical)

If this is to make the bitcasting hacks work, I think we should write them out by hand instead.


================
Comment at: clang/include/clang/AST/DependenceFlags.h:20
     Instantiation = 2,
-    Type = 4,
-    Value = 8,
+    Error = 4,
+    Type = 8,
----------------
sammccall wrote:
> Nit: it seems weird to jam this right in the middle of an enum.
> Instantiation/Type/Value are about template dependence, unexpandedpack is a bit different (but still strictly C++ semantics), Error is definitely different (outside language semantics).
> 
> Maybe make error the last element?
> (Making it first is weird as it's so atypical)
> 
> If this is to make the bitcasting hacks work, I think we should write them out by hand instead.
this needs a comment explaining what it is - we're making it up, so you can hardly look it up on cppreference :-)

Suggestion:
```This expr contains or references an error, and is considered dependent on how that error is resolved.```


================
Comment at: clang/include/clang/AST/DependenceFlags.h:46
     Instantiation = 2,
+    /// Placeholder, and is not used in actual Type.
+    Error = 4,
----------------
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.


================
Comment at: clang/lib/AST/ComputeDependence.cpp:626
   for (auto *A : E->arguments())
-    D |= A->getDependence() & ExprDependence::UnexpandedPack;
+    D |= A->getDependence() &
+         (ExprDependence::UnexpandedPack | ExprDependence::Error);
----------------
This pattern makes me nervous, and feel like we should be blacklisting bits rather than whitelisting them in general.

But no need to change it here, I think much of ComputeDependence could be written more expressively, project for another day.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65591