[PATCH] D83213: [AST][RecoveryExpr] Don't set the instantiation-bit.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/include/clang/AST/DependenceFlags.h:22
+    //     cause instantiation to fail
+    //   - or an error (usually in a non-template context)
+    //
----------------
nit: I'd weaken usually->often


================
Comment at: clang/include/clang/AST/DependenceFlags.h:25
+    // Note that C++ standard doesn't define the instantiation-dependent term,
+    // we follow the formal definition coming from the Itanium C++ ABI.
     Instantiation = 2,
----------------
Maybe add ", and extend it to errors"


================
Comment at: clang/include/clang/AST/DependenceFlags.h:55
+    /// Whether this type somehow involves
+    ///   - a template parameter, evenif the resolution of the type does not
+    ///     depend on a template parameter.
----------------
evenif > even if


================
Comment at: clang/include/clang/AST/DependenceFlags.h:62
+    ///   - or it somehow involves an error, e.g. denoted by
+    ///     decltype(recovery-expr) where recovery-expr is contains-errors
     Dependent = 4,
----------------
nit: i'd just say "e.g. decltype(expr-with-errors)"


================
Comment at: clang/include/clang/AST/DependenceFlags.h:112
     UnexpandedPack = 1,
     // Uses a template parameter, even if it doesn't affect the result.
+    // Validity depends on the template parameter, or an error.
----------------
I'd rephrase as "Depends on a template parameter or error in some way. Validity depends on how the template is instantiated or the error is resolved."


================
Comment at: clang/include/clang/AST/Expr.h:162
+  ///   - a template parameter (C++ [temp.dep.constexpr])
+  ///   - or an error
+  ///
----------------
maybe "or an error, whose resolution is unknown"

This hints at the connection between template and error dependency, and also is more accurate for type-dependence (where sometimes we're not type dependent because the type depends on an error but we've guessed at what the resolution is)


================
Comment at: clang/include/clang/AST/Expr.h:6233
+/// unlike other dependent expressions, RecoveryExpr can be produced in
+/// non-template contexts. In addition, we will preserve the type in
+/// RecoveryExpr when the type is known, e.g. preserving the return type for a
----------------
I'm not sure why this "in addition" is part of the same paragraph, it seems unrelated.

I'd move to a separate paragraph and drop "in addition".


================
Comment at: clang/include/clang/AST/Expr.h:6236
+/// broken non-overloaded function call, a overloaded call where all candidates
+/// have the same return type.
 ///
----------------
maybe "In this case, the expression is not type-dependent (unless the known type is itself dependent)"


================
Comment at: clang/lib/AST/ComputeDependence.cpp:499
+  // RecoveryExpr is
+  //   - always value-dependent, instantiation-dependent and contains-errors
+  //   - type-dependent if we don't know the type (fallback to an opequa
----------------
nit: I'd say "always value-dependent, and therefore instantiation dependent"
and make "contains-errors" a separate bullet at the end like "- contains errors (ExprDependence::Error), by definition"


================
Comment at: clang/lib/AST/ComputeDependence.cpp:500
+  //   - always value-dependent, instantiation-dependent and contains-errors
+  //   - type-dependent if we don't know the type (fallback to an opequa
+  //     dependent type), or the type is known and dependent, or it has
----------------
opaque


================
Comment at: clang/lib/AST/ComputeDependence.cpp:502
+  //     dependent type), or the type is known and dependent, or it has
+  //     type-dependent subexpressions
   auto D = toExprDependence(E->getType()->getDependence()) |
----------------
hmm, I'd missed the type-dependent subexpressions question.
If there are type-dependent subexpressions, but a non-dependent type was specified for the RecoveryExpr, is the expr type-dependent?

This is the part that I think we have discretion over.
The definition of type-dependent does say "any type-dependent subexpression" but then lays out a list of exceptions such as casts, which are not type-dependent even if their argument is. What these have in common is that the type is known.

So I think this comes down to whether it's the caller's job to work this out, or we want to conservatively call these expressions dependent.

I think the former is probably better - marking the expression as type-dependent but not having its actual type be dependent doesn't serve any purpose I'm aware of. It's also inconsistent with the informal definition of type-dependence described earlier in this patch.

So the comment should describe the current state, but maybe a FIXME to remove the type-dependent subexpressions clause?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83213