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