This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Support label at end of compound statement
ClosedPublic

Authored by Izaron on Sep 14 2022, 12:26 PM.

Diff Detail

Event Timeline

Izaron created this revision.Sep 14 2022, 12:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2022, 12:26 PM
Izaron requested review of this revision.Sep 14 2022, 12:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2022, 12:26 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I thought it was a good idea to slightly rename err_label_end_of_compound_statement to err_SWITCH_label_end_of_compound_statement because we aren't going to support empty labels in switch statements. (GCC also doesn't compile it - https://godbolt.org/z/xabKaon8v)

So now we need to clarify that we don't support empty labels in SWITCH compound statements (just there).

Thanks for doing that work.

Given that this paper was touted as a C compatibility feature, I think we should implement the C feature at the same time
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2508.pdf

If my understanding is correct the change implemented here should just be allowed - without warning in C23
What do you think @aaron.ballman ?

shafik added a subscriber: shafik.Sep 14 2022, 6:05 PM

The proposal says:

A label at the end of a compound statement is treated as if it were followed by a null statement

Does it make sense to write an AST test to verify this?

Thanks for doing that work.

Given that this paper was touted as a C compatibility feature, I think we should implement the C feature at the same time
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2508.pdf

If my understanding is correct the change implemented here should just be allowed - without warning in C23
What do you think @aaron.ballman ?

I don't think the two papers diverge in any way, so I think the implementation for one should basically cover the work for the other (aside from test cases, etc). It should definitely be without warning in C23 mode and give an extension warning in earlier modes.

The proposal says:

A label at the end of a compound statement is treated as if it were followed by a null statement

Does it make sense to write an AST test to verify this?

I think a test like that would be reasonable, yes.

Izaron updated this revision to Diff 460987.Sep 17 2022, 4:06 AM

Change C2x implementation status and C2x release notes

Add an AST test

It should definitely be without warning in C23 mode and give an extension warning in earlier modes.

@aaron.ballman we have this extension warning for pre-C++23:

def ext_label_end_of_compound_statement : ExtWarn<
  "label at end of compound statement is a C++2b extension">,
   InGroup<CXX2b>;

Should I add a new warning for pre-C23? We could have two warnings for each language:

ext_cxx_label_end_of_compound_statement
ext_c_label_end_of_compound_statement

It should definitely be without warning in C23 mode and give an extension warning in earlier modes.

@aaron.ballman we have this extension warning for pre-C++23:

def ext_label_end_of_compound_statement : ExtWarn<
  "label at end of compound statement is a C++2b extension">,
   InGroup<CXX2b>;

Should I add a new warning for pre-C23? We could have two warnings for each language:

ext_cxx_label_end_of_compound_statement
ext_c_label_end_of_compound_statement

I think that's reasonable, yes - given that we need different warning groups

Izaron updated this revision to Diff 461002.Sep 17 2022, 6:26 AM

Add diagnostics for C language

cor3ntin added inline comments.Sep 17 2022, 6:36 AM
clang/lib/Parse/ParseStmt.cpp
738

I do not understand this. Why checking getLangOpts().C99 instead of just C? Whyt no compatibility warning in C23 mode?
I think the code should look very similar for both C++ and C

Why checking getLangOpts().C99 instead of just C

There is no getLangOpts().C. Here are possible C/C++ opt flags:
https://github.com/llvm/llvm-project/blob/7914e53e312074828293356f569d190ac6eae3bd/clang/include/clang/Basic/LangOptions.def#L86-L100
I have no understanding why there is no getLangOpts().C flag. Maybe the C89 standard is a subset of all other C/C++/ObjC standards, so we don't need the flag?..

Whyt no compatibility warning in C23 mode

@aaron.ballman said so in https://reviews.llvm.org/D133887#3793027

It should definitely be without warning in C23 mode and give an extension warning in earlier modes.

I don't know much about extension/incompatible warnings policy (when to apply and not apply them), could you please help me to figure this out with Aaron? 😃

Why checking getLangOpts().C99 instead of just C

There is no getLangOpts().C. Here are possible C/C++ opt flags:
https://github.com/llvm/llvm-project/blob/7914e53e312074828293356f569d190ac6eae3bd/clang/include/clang/Basic/LangOptions.def#L86-L100
I have no understanding why there is no getLangOpts().C flag. Maybe the C89 standard is a subset of all other C/C++/ObjC standards, so we don't need the flag?..

I missed that.
Yes, i think the way do that is to check !getLangOpts().CPlusPlus

Whyt no compatibility warning in C23 mode

@aaron.ballman said so in https://reviews.llvm.org/D133887#3793027

It should definitely be without warning in C23 mode and give an extension warning in earlier modes.

I don't know much about extension/incompatible warnings policy (when to apply and not apply them), could you please help me to figure this out with Aaron? 😃

I checked with Aaron, he meant no warning _by default_, but there should definitely be an extension warning. exact same thing as C++.
Sorry for the confusion.

Izaron updated this revision to Diff 461006.Sep 17 2022, 7:43 AM

Fix diagnostics for C. Thanks to @cor3ntin and @aaron.ballman!

Izaron marked an inline comment as done.Sep 17 2022, 7:44 AM
cor3ntin accepted this revision as: cor3ntin.Sep 17 2022, 7:59 AM

Thanks.
This LGTM now

This revision is now accepted and ready to land.Sep 17 2022, 7:59 AM
This revision was landed with ongoing or failed builds.Sep 17 2022, 8:35 AM
This revision was automatically updated to reflect the committed changes.

Thank you for working on this! I spotted an issue where we're not quite complete with this implementation work. I pushed up a new test case in https://github.com/llvm/llvm-project/commit/a244194f73788de6dfd6d753ff09be3625613f9f that shows it (and set the C status page back to Partial in case this takes a while to address), but case and default labels should be accepted the same as any other label at the end of a compound statement.

Btw, when working on C standards features, I've started adding test coverage that demonstrates we fully implement the paper into the clang/test/C/ directory, under the correct standard version. This helps us to track the status of proposals a bit more easily within the code base.

clang/include/clang/Basic/DiagnosticParseKinds.td
298–312

This is not an error in C2x or in C++2b.

Thank you for working on this! I spotted an issue where we're not quite complete with this implementation work. I pushed up a new test case in https://github.com/llvm/llvm-project/commit/a244194f73788de6dfd6d753ff09be3625613f9f that shows it (and set the C status page back to Partial in case this takes a while to address), but case and default labels should be accepted the same as any other label at the end of a compound statement.

Btw, when working on C standards features, I've started adding test coverage that demonstrates we fully implement the paper into the clang/test/C/ directory, under the correct standard version. This helps us to track the status of proposals a bit more easily within the code base.

Hi! Thanks for this information! How did you find out that we should accept labels at end of statement in C, but not in C++? That is not obvious for me unfortunately. If C supports it, why would C++ not support it?

Thank you for working on this! I spotted an issue where we're not quite complete with this implementation work. I pushed up a new test case in https://github.com/llvm/llvm-project/commit/a244194f73788de6dfd6d753ff09be3625613f9f that shows it (and set the C status page back to Partial in case this takes a while to address), but case and default labels should be accepted the same as any other label at the end of a compound statement.

Btw, when working on C standards features, I've started adding test coverage that demonstrates we fully implement the paper into the clang/test/C/ directory, under the correct standard version. This helps us to track the status of proposals a bit more easily within the code base.

Hi! Thanks for this information! How did you find out that we should accept labels at end of statement in C, but not in C++? That is not obvious for me unfortunately. If C supports it, why would C++ not support it?

I knew the answer for C off the top of my head, but was still looking up the info for C++. C++ does actually behave the same way:

https://eel.is/c++draft/stmt.label#1 -- case and default are both a label
https://eel.is/c++draft/stmt.block#1.sentence-2 -- all labels at the end of a compound statement are treated as if they are followed by a null statement
https://eel.is/c++draft/stmt.stmt#nt:selection-statement -- a switch is followed by a statement
https://eel.is/c++draft/stmt.stmt#stmt.pre-1 -- a compound-statement is a statement

I can fix up the C++ status page as well if you'd like, or I can hold off if you're already working on this, it's up to you!

I can fix up the C++ status page as well if you'd like, or I can hold off if you're already working on this, it's up to you!

Thank you! I'll support empty labels in switch statements in a new pull request as soon as I can 😁

I can fix up the C++ status page as well if you'd like, or I can hold off if you're already working on this, it's up to you!

Thank you! I'll support empty labels in switch statements in a new pull request as soon as I can 😁

Thank you, I appreciate it!