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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/include/clang/AST/DependenceFlags.h:46
     Instantiation = 2,
+    /// Placeholder, and is not used in actual Type.
+    Error = 4,
----------------
sammccall wrote:
> hokein wrote:
> > 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.
> > 
> > 
> I think we should plan to do this for Type too, though it's OK not to do so in this patch.
> 
> consider e.g. the expression `decltype(foo){}` where `foo` has errors. Today we're saying this has no errors, because the DeclTypeType node isn't error-dependent.
> 
> (This is true whether you add the enum value or not: the `Type` constructor takes a bunch of booleans in the constructor, it would need to be refactored to support `TypeDependence` and possibly the computeDependence pattern)
> 
> I think we should ensure that as far as possible, this code conceptually propagates error-dependence from types to expressions correctly, even if the error-dependence is not set in practice yet.
> I'm not sure if this requires having the Error bit in `TypeDependence` now: if we never have to name it because we only blacklist bits, then it's probably OK.
OK, I'd prefer to not expand the scope of this patch right now, because we don't have proper tests :( until the final recovery expression patch landed. and I think adding the error-bit to Type would probably require more effort (more code changes).

updated the code in `computeDependency` to make sure the error-bit will be propagated from type-dependence to err-dependence  even we don't have the error bit in TypeDependency now.


>(This is true whether you add the enum value or not: the Type constructor takes a bunch of booleans in the constructor, it would need to be refactored to support TypeDependence and possibly the computeDependence pattern)

agree, I think the same to the NestedNameSpecifierDependence, TemplateNameDependence, TemplateArgumentDependence.


================
Comment at: clang/lib/AST/ComputeDependence.cpp:174
 ExprDependence clang::computeDependence(NoInitExpr *E) {
   return toExprDependence(E->getType()->getDependence()) &
          ExprDependence::Instantiation;
----------------
sammccall wrote:
> I'm not terribly sure of the implications of not propagating the error bit here. I tend to think that "contains errors" most closely follows instantiation-dependence (i.e. it's fairly strict/lexical), so I'd consider propagating it here.
> 
> BTW, DesignatedInitUpdateExpr seems to have totally broken dependence computations - it's always non-dependent! (Not introduced by this refactoring, I think). Any idea what's up there?
you mean `DesignatedInitExpr`? I didn't see any problem there, why it is always non-dependent?


================
Comment at: clang/lib/AST/ComputeDependence.cpp:500
   if (E->isResultDependent())
     return D | ExprDependence::TypeValueInstantiation;
   return D | (E->getResultExpr()->getDependence() &
----------------
sammccall wrote:
> this should be D |=... so that result expr errors propagate
getResultExpr() requires `! isResultDependent()`...

and `D |=` is not enough, for err-bit, I think we need to consider all subexpressions. 


================
Comment at: clang/lib/AST/ComputeDependence.cpp:582
     Deps |= toExprDependence(Q->getDependence());
   for (auto *D : E->decls()) {
     if (D->getDeclContext()->isDependentContext() ||
----------------
sammccall wrote:
> (if we decide to make referring to an `invalid` decl an error, this is another spot... there are probably lots :-(. Deferring this is probably best)
deferred this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65591