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