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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


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


================
Comment at: clang/lib/Parse/ParseCXXInlineMethods.cpp:398
         DefArgResult = ParseAssignmentExpression();
-      DefArgResult = Actions.CorrectDelayedTyposInExpr(DefArgResult);
+      DefArgResult = Actions.CorrectDelayedTyposInExpr(DefArgResult, Param);
       if (DefArgResult.isInvalid()) {
----------------
sammccall wrote:
> If this fixes the recursive copy-constructor case you mentioned, can you add a test?
> 
> (Else does it do anything? Or just cleanup)
no it doesn't. unfortunately that requires change in a separate code path to mark any method decls with invalid parmvardecls as invalid, which i'll try to put together. as that's the behavior for regular functiondecls, i don't see why it should be different for methods (if so we should probably change the behavior for functions instead).


================
Comment at: clang/lib/Parse/ParseCXXInlineMethods.cpp:400
       if (DefArgResult.isInvalid()) {
-        Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
+        Actions.ActOnParamDefaultArgumentError(Param, EqualLoc, DefArgResult);
       } else {
----------------
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".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157868