hokein added inline comments.
================
Comment at: clang/include/clang/AST/Expr.h:4081
- : Expr(ConvertVectorExprClass, DstType, VK, OK,
- DstType->isDependentType(),
- DstType->isDependentType() || SrcExpr->isValueDependent(),
----------------
sammccall wrote:
> hokein wrote:
> > ! behavior change change, now the `ConvertVectorExpr::TypeDependent =
> > DstType->isDependentType() | SrcExpr->isDependentType()`.
> this looks incorrect to me. A conversion's type doesn't depend on the input
> type.
reverted to the old behavior.
================
Comment at: clang/include/clang/AST/Expr.h:4250
SourceLocation RPLoc, QualType t, bool IsMS)
- : Expr(VAArgExprClass, t, VK_RValue, OK_Ordinary, t->isDependentType(),
- false, (TInfo->getType()->isInstantiationDependentType() ||
----------------
sammccall wrote:
> hokein wrote:
> > ! the implementation in the original patch doesn't seem to correct to me, I
> > rewrote it, need another review..
> Are you saying the previous version of this patch didn't match the spec, or
> didn't match the old behavior, or both?
>
> The new one seems pretty different to head.
> e.g. parameter packsand instantiation dependence are propagated from type,
> type dependence is propagated from the subexpr and the writtentypeinfo's
> type.
>
> Do these not make a difference for practical cases?
> (If this is a bug fix, I'd prefer to land it separately)
ah, you are right, I intended to make the new implementation match the old
behavior (not sure what I thought before), updated, it should match the old
behavior now.
================
Comment at: clang/include/clang/AST/Expr.h:5031
- CommonInit->isValueDependent() || ElementInit->isValueDependent(),
- T->isInstantiationDependentType(),
- CommonInit->containsUnexpandedParameterPack() ||
----------------
sammccall wrote:
> hokein wrote:
> > ! behavior change here, now `ArrayInitLoopExpr::Instantiation =
> > T->isInstantiationDependentType() |
> > CommonInit->isInstantiationDependentType() |
> > ElementInit->isInstantiationDependentType()`.
> Is this a bugfix? Consider leaving a FIXME instead?
reverted to old behavior.
================
Comment at: clang/include/clang/AST/Expr.h:5650
- : Expr(AsTypeExprClass, DstType, VK, OK,
- DstType->isDependentType(),
- DstType->isDependentType() || SrcExpr->isValueDependent(),
----------------
sammccall wrote:
> hokein wrote:
> > ! behavior change here, now `Expr::TypeDependent =
> > DstType->isDependentType() | SrcExpr->isTypeDependent()`.
> This seems incorrect to me. Like a new-expr, the type of the result doesn't
> depend on the type of the inputs.
sounds fair, reverted to old behavior.
================
Comment at: clang/include/clang/AST/ExprCXX.h:4102
std::uninitialized_copy(PartialArgs.begin(), PartialArgs.end(), Args);
+ setDependence(Length ? ExprDependence::None
+ : ExprDependence::ValueInstantiation);
----------------
sammccall wrote:
> Why not have a computeDependence function for this?
> In particular we'll end up missing haserrors propagation without it, right?
this looks like a very straight-forward and simple `Dependence` initialization,
and it doesn't depend on other elements
For this particular case, I don't see big difference of having a
`computeDependence` function. If we introduce the error bit, we probably just
set it to false' but for `CXXFoldExpr` example, it makes sense to have a
`computeDependence` function, we should populate the errorbit from its
subexprs.
and this patches has other places where we directly initialize the dependence,
we should have `computeDependence` for all of them?
================
Comment at: clang/include/clang/AST/TemplateBase.h:672
TemplateArgumentLoc *OutArgArray);
+ // FIXME: cleanup the unsued Deps.
void initializeFrom(SourceLocation TemplateKWLoc,
----------------
sammccall wrote:
> I don't understand this comment, can you expand it?
The parameter `Deps` is the result populated by this method, the caller doesn't
need it since we have the `computeDependencies`.
================
Comment at: clang/lib/AST/ComputeDependence.cpp:13
+#include "clang/AST/DeclarationName.h"
+#include "clang/AST/DependencyFlags.h"
+#include "clang/AST/Expr.h"
----------------
sammccall wrote:
> doh, we forgot to fix dependencyflags -> dependenceflags in the previous patch
will rename it in a separate patch.
================
Comment at: clang/lib/AST/ComputeDependence.cpp:24
+
+// In Expr.h
+ExprDependence clang::computeDependence(FullExpr *E) {
----------------
sammccall wrote:
> I'm not sure who these comments are for.
These were for code reviews, functions below were in the `Expr.h` before this
patch. I will remove them when landing this patch.
================
Comment at: clang/lib/AST/ComputeDependence.cpp:54
+ auto ArgDeps = E->getArgumentExpr()->getDependence();
+ auto Deps = ArgDeps & ~ExprDependence::TypeValue;
+ // Value-dependent if the argument is type-dependent.
----------------
sammccall wrote:
> and again here
this is not exactly the same (by changing the Deps to
`turnTypeToValueDependence(ArgDeps)`), we don't set the value-dependent to Deps
if the ArgDeps is value-dependent but not type-dependent.
`sizeof(unary-expression)` is value-dependent if unary-expression is
type-dependent, consider the expression `sizeof(sizeof(unary-expression))` and
unary-expression is type-dependent, the inner `sizeof` is value-dependent but
not type-dependent, and outer `sizeof` is not value-dependent.
================
Comment at: clang/lib/AST/ComputeDependence.cpp:285
+ return turnTypeToValueDependence(E->getQueriedExpression()->getDependence())
&
+ ~ExprDependence::Type;
+}
----------------
sammccall wrote:
> this seems redundant, you need the turn OR the ~type
oops, this didn't match the old behavior. `E` is value-dependent if the
queried expression is type-dependent. Fixed.
================
Comment at: clang/lib/AST/ComputeDependence.cpp:363
+/// based on the declaration being referenced.
+ExprDependence clang::computeDependence(DeclRefExpr *E, const ASTContext &Ctx)
{
+ auto Deps = ExprDependence::None;
----------------
sammccall wrote:
> I'm not really sure why we're changing this. The new version doesn't seem
> obviously clearer or faster. Is it a bug fix?
not this is not a bug fix, it just moved the implementation from the `Expr.cpp`
to here, are you suggesting to keep it in `Expr.cpp`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73638/new/
https://reviews.llvm.org/D73638
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
