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

Erich Keane via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Apr 4 06:41:34 PDT 2022

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'.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list