[PATCH] D119609: [Clang][Sema] Prohibit expr statement in the default argument

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D119609#3424434 <https://reviews.llvm.org/D119609#3424434>, @junaire wrote:

> Hi @aaron.ballman, do you think it's time to consider reviewing this?
>
> I don't why there's no response or progress in GCC, but I think I can submit a patch for them if they think this issue is low prior.

I think I'm ok with rejecting both cases now, and doing this review.  Though I'm still concerned with the 'skip-until' losing some diagnostics, so please add another test for that as I requested before.



================
Comment at: clang/test/Sema/err-expr-stmt-in-default-arg.cpp:3
+
+void foo() {
+  void fn(int i, int = ({ 1; })); // expected-error {{expression statement not permitted in default argument}}
----------------
Do we also prohibit in a template argument?


================
Comment at: clang/test/Sema/err-expr-stmt-in-default-arg.cpp:19
+  return bar(l);
+}
----------------
erichkeane wrote:
> junaire wrote:
> > erichkeane wrote:
> > > For completeness, I'd like to see an in-function-defined-struct-member-function test here as well.
> > > 
> > > As for the above question about the recovery, something like:
> > > 
> > > `void fn(int i, int j = ({ {},{},{,} }), int k = "");` I think checks all the issues I can think of.  We want to make sure that 'k' still diagnoses its error correctly (and that we have just skipped all of the expression statement stuff).
> > Note that clang is already rejected the code: https://godbolt.org/z/57c4qaaPo
> > 
> I more mean an example like this one:
> https://godbolt.org/z/nf7W1zznh
> 
> I see that we already reject it, however you are doing a 'skip until' here, and I want to make sure that the error on 'k' diagnoses correctly still. 
> 
> With your change, I would expect the 1st two errors there to go away and be replaced by your new error.  BUT the 'k' type would still be there.
> 
> The point of this is to make sure that your error doesn't leave the parser in a 'bad' state.
I don't see the test I requested here, I'd still like to see that to make sure the parser still diagnoses' k'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119609