[PATCH] D84304: [AST][RecoveryExpr] Part 2: Build dependent callexpr in C for error-recovery.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:6437
 
+  if (getLangOpts().CDependence &&
+      (Fn->isTypeDependent() || Expr::hasAnyTypeDependentArguments(ArgExprs))) {
----------------
why is CDependence a condition and not an assertion inside the if?
(If it's for performance, then we're going to lose this performance when flipping the flag, so I'd consider just flipping it now)

If you're using it as a short-hand for "is this plain C and also has recovery enabled" then I'd avoid that - it's not clear that this is a language check.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:6444
+               "should only occur in error-recovery path.");
+    return CallExpr::Create(Context, Fn, ArgExprs, Context.DependentTy,
+                            VK_RValue, RParenLoc);
----------------
Is this really the right place vs in BuildResolvedCallExpr?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:6444
+               "should only occur in error-recovery path.");
+    return CallExpr::Create(Context, Fn, ArgExprs, Context.DependentTy,
+                            VK_RValue, RParenLoc);
----------------
sammccall wrote:
> Is this really the right place vs in BuildResolvedCallExpr?
why DependentTy? shouldn't it be the return type of the function, if available?


================
Comment at: clang/test/AST/ast-dump-recovery.c:70
+  // in Sema::CheckPlaceholderExpr.
+  // CHECK-NOT: DeclRefExpr {{.*}} 'ext' 'int (int)'
+  ext(undef_var);
----------------
i'm not sure what the `int (int)` is about - can you remove it from the assertion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84304