[PATCH] D157868: [clang] Use RecoveryExprs for broken defaultargs, instead of OpaqueValueExprs

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Still LG once you're happy



================
Comment at: clang/include/clang/Sema/Sema.h:3026
+  void ActOnParamDefaultArgumentError(Decl *param, SourceLocation EqualLoc,
+                                      ExprResult DefaultArg);
   ExprResult ConvertParamDefaultArgument(ParmVarDecl *Param, Expr *DefaultArg,
----------------
kadircet wrote:
> sammccall wrote:
> > nit: Nullable `ExprResult*` since there are only two states?
> > Extra get() in some callers, but less mysterious
> i guess you meant `Expr*` ?
Yes, sorry!


================
Comment at: clang/lib/Parse/ParseCXXInlineMethods.cpp:400
       if (DefArgResult.isInvalid()) {
-        Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
+        Actions.ActOnParamDefaultArgumentError(Param, EqualLoc, DefArgResult);
       } else {
----------------
kadircet wrote:
> sammccall wrote:
> > DefArgResult is never anything here. I'd probably find `/*DefaultArg=*/nullptr` more obvious
> maybe i am missing something, but "invalid" doesn't mean "unusable".
ActionResult can be pointer-and-valid, null-and-valid, or null-and-invalid.

So invalid does indeed mean unusable ("usable" means pointer-and-valid).

Its documentation strongly suggests an ActionResult can be pointer-and-invalid, but good luck constructing one :-)


================
Comment at: clang/test/AST/ast-dump-default-arg-recovery.cpp:6
+//      CHECK: -ParmVarDecl {{.*}} <col:10, col:24> col:14 invalid arg 'int' cinit
+// CHECK-NEXT:   -RecoveryExpr {{.*}} <col:18, col:24> 'int' contains-errors
----------------
Can you also check the contents (there should be a declrefexpr at least)?

Preserving this internal structure is important for clangd, the recoveryexpr alone doesn't help much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157868