[PATCH] D73638: [AST] Move dependence computations into a separate file

[Haojian Wu via Phabricator via cfe-commits]

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