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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

So this patch makes me pretty nervous - it moves a lot of logic, it necessarily changes the way it's structured, the domain is subtle. I wouldn't be terribly surprised if we were missing some tests here.
So this all points to resisting the urge to improve things in this patch, and doing it in followups instead.



================
Comment at: clang/include/clang/AST/Expr.h:4081
-    : Expr(ConvertVectorExprClass, DstType, VK, OK,
-           DstType->isDependentType(),
-           DstType->isDependentType() || SrcExpr->isValueDependent(),
----------------
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.


================
Comment at: clang/include/clang/AST/Expr.h:4139
-             bool TypeDependent, bool ValueDependent)
-    : Expr(ChooseExprClass, t, VK, OK, TypeDependent, ValueDependent,
-           (cond->isInstantiationDependent() ||
----------------
hokein wrote:
> ! this needs careful review, the ChooseExpr bits are from different sources:
> 
> - constructor here
> - [`clang/lib/Sema/SemaPseudoObject.cpp`](https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaPseudoObject.cpp#L170)
> - [`clang/lib/AST/ASTImporter.cpp`](https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ASTImporter.cpp#L6347)
> - [`clang/lib/Sema/SemaExpr.cpp`](https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaExpr.cpp#L14207)
> 
>  
LG


================
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() ||
----------------
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)


================
Comment at: clang/include/clang/AST/Expr.h:5031
-             CommonInit->isValueDependent() || ElementInit->isValueDependent(),
-             T->isInstantiationDependentType(),
-             CommonInit->containsUnexpandedParameterPack() ||
----------------
hokein wrote:
> ! behavior change here, now `ArrayInitLoopExpr::Instantiation =  T->isInstantiationDependentType() |  CommonInit->isInstantiationDependentType() |  ElementInit->isInstantiationDependentType()`.
Is this a bugfix? Consider leaving a FIXME instead?


================
Comment at: clang/include/clang/AST/Expr.h:5650
-    : Expr(AsTypeExprClass, DstType, VK, OK,
-           DstType->isDependentType(),
-           DstType->isDependentType() || SrcExpr->isValueDependent(),
----------------
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.


================
Comment at: clang/include/clang/AST/Expr.h:152
 
   void setDependence(ExprDependence Deps) {
     ExprBits.Dependent = static_cast<unsigned>(Deps);
----------------
This should have some docs like "each concrete expr subclass is expected to compute its dependence and call this in the constructor".


================
Comment at: clang/include/clang/AST/ExprCXX.h:2742
-      : Expr(ArrayTypeTraitExprClass, ty, VK_RValue, OK_Ordinary,
-             false, queried->getType()->isDependentType(),
-             (queried->getType()->isInstantiationDependentType() ||
----------------
hokein wrote:
> ! behavior change here, now we the `ValueDependent`, `UnexpandedPack` takes `dimension` into account as well.
this seems like a reasonable/trivial bugfix


================
Comment at: clang/include/clang/AST/ExprCXX.h:4102
     std::uninitialized_copy(PartialArgs.begin(), PartialArgs.end(), Args);
+    setDependence(Length ? ExprDependence::None
+                         : ExprDependence::ValueInstantiation);
----------------
Why not have a computeDependence function for this?
In particular we'll end up missing haserrors propagation without it, right?


================
Comment at: clang/include/clang/AST/TemplateBase.h:672
                       TemplateArgumentLoc *OutArgArray);
+  // FIXME: cleanup the unsued Deps.
   void initializeFrom(SourceLocation TemplateKWLoc,
----------------
I don't understand this comment, can you expand it?


================
Comment at: clang/lib/AST/ComputeDependence.cpp:13
+#include "clang/AST/DeclarationName.h"
+#include "clang/AST/DependencyFlags.h"
+#include "clang/AST/Expr.h"
----------------
doh, we forgot to fix dependencyflags -> dependenceflags in the previous patch


================
Comment at: clang/lib/AST/ComputeDependence.cpp:24
+
+// In Expr.h
+ExprDependence clang::computeDependence(FullExpr *E) {
----------------
I'm not sure who these comments are for.


================
Comment at: clang/lib/AST/ComputeDependence.cpp:50
+  if (E->isArgumentType())
+    return toExprDependence(E->getArgumentType()->getDependence()) &
+           ~ExprDependence::Type;
----------------
this is turnTypeToValueDependence(...)


================
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.
----------------
and again here


================
Comment at: clang/lib/AST/ComputeDependence.cpp:165
+ExprDependence clang::computeDependence(VAArgExpr *E) {
+  return (toExprDependence(E->getType()->getDependence()) |
+          E->getSubExpr()->getDependence() |
----------------
apart from any correctness issues, I think this expression is too complicated and should be broken down.


================
Comment at: clang/lib/AST/ComputeDependence.cpp:280
+    D |= Dim->getDependence();
+  return D & ~ExprDependence::Type;
+}
----------------
might be clearer as turnTypeToValueDependence, I think that's conceptually what's going on


================
Comment at: clang/lib/AST/ComputeDependence.cpp:285
+  return turnTypeToValueDependence(E->getQueriedExpression()->getDependence()) &
+         ~ExprDependence::Type;
+}
----------------
this seems redundant, you need the turn OR the ~type


================
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;
----------------
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?


================
Comment at: clang/lib/AST/ComputeDependence.cpp:461
+  for (auto *A : llvm::makeArrayRef(E->getArgs(), E->getNumArgs())) {
+    if (A == nullptr)
+      continue;
----------------
nit: `if (A) D|= ...` ?


================
Comment at: clang/lib/AST/ExprCXX.cpp:444
-          SC, Context.OverloadTy, VK_LValue, OK_Ordinary, KnownDependent,
-          KnownDependent,
-          (KnownInstantiationDependent || NameInfo.isInstantiationDependent() ||
----------------
hokein wrote:
> ! this change is not trivial, need an review.
LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73638