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