C++17 feature proposal: [[fallthrough]]

[Roland Bock]

Hi,

The [[fallthrough]] attribute is great for documenting intentional fall-through in a switch statement (as opposed to a forgotten break or return).

It can also be combined with -Wimplicit-fallthrough in clang and gcc to enforce at some point.

Therefore, I nominate [[fallthrough]] from C++17 to be allowed.

Best,

Roland

[Peter Kasting]

We currently use [[clang:fallthrough]], behind the macro FALLTHROUGH, for this purpose (and enforce via the warning you mention).  Presumably, then, this proposal would be to either redefine the macro to just [[fallthrough]] and use it on all compilers, or to also go further and replace use of the macro with direct use of this attribute?

PK

[Avi Drissman]

I would support a version of this proposal that called for the removal of the macro FALLTHROUGH and the substitution of [[fallthrough]] everywhere.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFD%3D%2BwwML-BwGRYfcs11wfKgpP6rshhPXaEthM_H0PiD-w%40mail.gmail.com.

[K. Moon]

+1 to fewer macros whenever possible.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAaVkajavRL8YSpeAopeNH%2Ba2iKLnk9c0GY2n-wxk7vhcg%40mail.gmail.com.

[Roland Bock]

I was not even aware of the macro (and so weren't others) and it does not seem to be enforced, see this review, for instance :-)

So the proposal would be to 

• remove the FALLTHROUGH macro and substitute it with [[fallthrough]] everywhere,
  
• develop a plan to introduce the -Wimplicit-fallthrough warning at least for our own code.

[Lei Zhang]

On Fri, Dec 24, 2021 at 9:05 AM 'Roland Bock' via cxx <c...@chromium.org> wrote:
> develop a plan to introduce the -Wimplicit-fallthrough warning at least for our own code.

Already in build/config/compiler/BUILD.gn for chromium_code. Done!

[Roland Bock]

Ha, I could have sworn this would warn for something like

switch(x)

  case 0:

  case 1: break;

}

Well, even better :-)

[Avi Drissman]

For the proposal of allowing [[fallthrough]] and doing the conversion of FALLTHROUGH, +1.

This should be a pretty straightforward LSC.

[Peter Kasting]

On Fri, Dec 24, 2021 at 9:54 AM Avi Drissman <a...@google.com> wrote:

For the proposal of allowing [[fallthrough]] and doing the conversion of FALLTHROUGH, +1.

+1

PK 

[Jeremy Roman]

I think it's correct for it not to be enforced there. The case of fallthrough between switch cases where there's no code between the case labels is incredibly common and IMO very clear. The problematic case [[fallthrough]] handles is where there is some statement between the two case labels and it's not obvious whether fallthrough was intended or "break;" was forgotten.

i.e. we should write

case 1:

  Foo();

  [[fallthrough]];

case 2:

  Bar();

  break;

but

case 1:

case 2:

  Foo();

  break;

I believe this is what -Wimplicit-fallthrough already demands.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHq1K7ShAcrJbhx7BvGm-0jrRSJTeYtVPNhatZfjSQWm%2Ba5U%3DQ%40mail.gmail.com.

[Peter Kasting]

On Fri, Dec 24, 2021 at 9:54 AM Avi Drissman <a...@google.com> wrote:

For the proposal of allowing [[fallthrough]] and doing the conversion of FALLTHROUGH, +1.

This seems pretty non-controversial, so I suggest we consider it approved.  It seems like there are several parts:

(1) Move the [[fallthrough]] attribute to the allowed section of the guide

(2) Write an LSC proposal to search-and-replace FALLTHROUGH with [[fallthrough]]

(3) Do the LSC and eliminate the macro

Roland, what parts of this are you signing up for?  (1), or all three?

PK

[Roland Bock]

I can do (1) for sure.

I am happy to give (2) and (3) a try, too, following https://chromium.googlesource.com/chromium/src/+/HEAD/docs/process/lsc/large_scale_changes.md, I guess.

[Roland Bock]

Happy New Year!

Here is the CL to allow [[fallthrough]].

About the LSC:

Replacing FALLTHROUGH with [[fallthrough]] looks straight forward in most places.

However, I am not sure how to deal with code in third_party/? There seem to be two cases to deal with:

• pdfium comes with it's own copy of third_party/pdfium/third_party/base/compiler_specific.h
• everything else is using base/compiler_specific.h, including some generated code for blink

We probably should change the code in third_party/, too? 

Would we inform the respective owners (how)?

What about pdfium? Leave as is, or change it, too?

Thanks!

Roland

[Avi Drissman]

PDFium has their own base/ that they do their best to keep in sync; for that, we'd file a bug against them and let them handle it.

For Blink generated code, we would update the generators to no longer use FALLTHROUGH and regenerate the code.

For third_party, you'll need to go case-by-case and figure out how to do it. For some of them, it's in the integration code and you can just change it in the tree. For others, you might need to change it upstream and re-roll the DEPS. I've done this before, so I can give guidance if necessary.

In the past, I've found it useful to do a burndown. Start with the LSC across Chromium and Blink, and let's see what's left.

[Lei Zhang]

File a bug on https://crbug.com/pdfium/new please.

> --
> You received this message because you are subscribed to the Google Groups "cxx" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

> To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHq1K7QaEdYBSa3ssfknq0BQMze_ZeFjEKTswMShceby-2zncg%40mail.gmail.com.

[Roland Bock]

Whoops! I filled out the LSC template which asked how many CLs  `git cl split` would produce.

Turns out 168  of them (for non-third-party code), which are not live in gerrit.

That was not really intended to happen yet. Fortunately, they are all super simple.

[Raphael Kubo Da Costa]

I don't have access to https://crbug.com/1283907 -- hopefully this change is not that controversial :-) Did it get some extra protection labels because it was filed by someone with a google.com email address?

[dan...@chromium.org]

Seems so, fixed it.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/a466b86a-a6bf-4933-9666-65e2c063ff28n%40chromium.org.

[Roland Bock]

Just for completeness:

The FALLTHROUGH macro is now gone.

Thanks to everyone who helped with the various CLs on the way :-)

Cheers,

Roland