On Fri, Oct 13, 2023 at 10:56:27AM +0300, Emmanouil Pitsidianakis wrote: > /* resubmitted because git-send-email crashed with previous attempt */ > > Hello, > > This RFC is inspired by the kernel's move to -Wimplicit-fallthrough=3 > back in 2019.[0] > We take one step (or two) further by increasing it to 5 which rejects > fall through comments and requires an attribute statement. > > [0]: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b > > The line differences are not many, but they spread all over different > subsystems, architectures and devices. An attempt has been made to split > them in cohesive patches to aid post-RFC review. Part of the RFC is to > determine whether these patch divisions needs improvement. > > Main questions this RFC poses > ============================= > > - Is this change desirable and net-positive.
I think so - consistency eases code maintenance, and being able to define a keyword-like macro used like any other control-flow statement is nicer than a magic comment. > - Should the `fallthrough;` pseudo-keyword be defined like in the Linux > kernel, or use glib's G_GNUC_FALLTHROUGH, or keep the already existing > QEMU_FALLTHROUGH macro. This seems like it only affects the one place where we define the keyword. As long as all switch statements actually using it stick to one style, I'm less concerned about the magic used to get the style working in the first place. > - Should fallthrough comments be removed if they do not include extra > information. That would be fine by me - but we'll see what other reviewers say. I'm going to review on just the files I normally touch. > > Some external resources > ======================= > > See the RFC discussion in the kernel: > > https://lore.kernel.org/lkml/1d2830aadbe9d8151728a7df5b88528fc72a0095.1564549413.git....@perches.com/ > > The `fallthrough;` pseudo-keyword in the kernel source code: > > https://elixir.bootlin.com/linux/latest/C/ident/fallthrough > > In summary, I quote the doc comment and definition: > > /* > * Add the pseudo keyword 'fallthrough' so case statement blocks > * must end with any of these keywords: > * break; > * fallthrough; > * continue; > * goto <label>; > * return [expression]; > * > * gcc: > https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes > */ > #if __has_attribute(__fallthrough__) > # define fallthrough __attribute__((__fallthrough__)) > #else > # define fallthrough do {} while (0) /* fallthrough */ > #endif > > Background - Motivation > ======================= > > The C switch statement allows you to conditionally goto different labels > depending on a value. A break; statement conveniently goto's the end of > the switch. If a "case" does not end in a break, we say that the control > flow falls through the next case label, if any, implicitly. This can > lead to bugs and QEMU uses the GCC warning -Wimplicit-fallthrough to > prevent this. > > Currently, QEMU is built with -Wimplicit-fallthrough=2. This makes GCC's > static analyzer check for a case-insensitive matches of the .*falls?[ > \t-]*thr(ough|u).* regular expression. This means the following list of > comments taken from QEMU all disable the implicit fallthrough warning: > > - /* FALLTHRU */ > - /* fall through */ > - /* Fall through. */ > - /* Fall through... */ > - /* fall through if hEventTimeout is signaled */ > - /* FALL THROUGH */ > > To keep a constistent code style, this commit adds a macro `fallthrough` > that looks like a C keyword but expands to an attribute statement in > supported compilers (GCC at the moment). > > Note: there was already such a macro, QEMU_FALLTHROUGH, and it was used > only around 7 times in the code base. The first commit replaces it. Seems reasonable to me; we'll see what other comments you get. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
