rsmith added inline comments.
================
Comment at: include/clang/AST/ExprCXX.h:4237
+ /// compiler.
+ bool IsImplicitlyCreated : 1;
+
----------------
I would go with just `isImplicit`, to match other similar uses throughout the
AST. Also maybe sink this into the `Stmt` bitfields to make this class 8 bytes
smaller
================
Comment at: lib/AST/ItaniumMangle.cpp:3302-3303
case Expr::AddrLabelExprClass:
+ // This should no longer exist in the AST by now
+ case Expr::DependentCoawaitExprClass:
case Expr::DesignatedInitUpdateExprClass:
----------------
I don't think "should no longer exist" is true. If `co_await` can appear in a
function signature at all, it can appear with a dependent operand. This should
be mangled the same as a non-dependent `co_await` expression.
================
Comment at: lib/Sema/SemaCoroutine.cpp:33
/// function type.
static QualType lookupPromiseType(Sema &S, const FunctionProtoType *FnType,
+ SourceLocation KwLoc,
----------------
EricWF wrote:
> The changes to this function are all unrelated cleanup/improvements.
Just go ahead and commit these separately then.
================
Comment at: lib/Sema/SemaCoroutine.cpp:99
+
+ auto buildNNS = [&]() {
auto *NNS = NestedNameSpecifier::Create(S.Context, nullptr, StdExp);
----------------
`buildElaboratedType` would be a better name for what this does. I also wonder
if this is really necessary, or whether we can just use %q0 in the diagnostic
format to request a fully-qualified type name.
================
Comment at: lib/Sema/SemaCoroutine.cpp:112-116
+ if (S.RequireCompleteType(FuncLoc, buildNNS(),
+ diag::err_coroutine_promise_type_incomplete))
+ return QualType();
return PromiseType;
----------------
We used to return the `ElaboratedType` and don't do so any more.
================
Comment at: lib/Sema/SemaCoroutine.cpp:409-414
+ return BuildDependentCoawaitExpr(Loc, E,
+ cast<UnresolvedLookupExpr>(Lookup.get()));
+}
+
+ExprResult Sema::BuildDependentCoawaitExpr(SourceLocation Loc, Expr *E,
+ UnresolvedLookupExpr *Lookup) {
----------------
This seems like an odd naming choice. I'd expect `BuildDependentCoawaitExpr` to
only deal with the case where the expression is dependent (and to never be
called otherwise), and `BuildCoawaitExpr` to handle both the case where the
expression is dependent and the case where it is non-dependent. Maybe the other
function should be called `BuildResolvedCoawaitExpr` or similar.
================
Comment at: lib/Sema/SemaDecl.cpp:11386
- // If we don't have a visible definition of the function, and it's inline or
+ // If we don't have a viNsible definition of the function, and it's inline or
// a template, skip the new definition.
----------------
Stray change.
================
Comment at: lib/Sema/TreeTransform.h:6687-6689
+ // Set that we have (possibly-invalid) suspend points before we do anything
+ // that may fail.
+ ScopeInfo->setCoroutineSuspendsInvalid();
----------------
Please use a term other than "invalid" here. We generally use "invalid" to mean
"an error has been diagnosed, use best-effort recovery".
================
Comment at: lib/Sema/TreeTransform.h:6802
+ return getDerived().RebuildDependentCoawaitExpr(
+ E->getKeywordLoc(), Result.get(), E->getOperatorCoawaitLookup());
+}
----------------
You need to transform the UnresolvedLookupExpr here too. (Example: we might
have a function-local declaration of `operator co_await` that needs to be
transformed into the version in the instantiated function.)
https://reviews.llvm.org/D26057
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
