Re: [RFC PATCH 00/78] Strict disable implicit fallthrough

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Oct 13, 2023 at 03:51:22PM +0300, Manos Pitsidianakis wrote:
> On Fri, 13 Oct 2023 11:14, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
> > On Fri, Oct 13, 2023 at 10:47:04AM +0300, Emmanouil Pitsidianakis wrote:
> > > 
> > > Main questions this RFC poses
> > > =============================
> > > 
> > > - Is this change desirable and net-positive.
> > 
> > Yes, IMHO it is worth standardizing on use of the attribute. The allowed
> > use of comments was a nice thing by the compiler for coping with pre-existing
> > code, but using the attribute is best long term for a consistent style.
> > 
> > > - 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.
> > 
> > As a general rule, if glib provides functionality we aim o use that
> > and not reinvent the wheel. IOW, we should just use G_GNUC_FALLTHROUGH.
> 
> I agree. My reasoning was:
> 
> - The reinvented wheel is only an attribute and not a big bunch of NIH  code

It isn't just about the amount of code, it is the familiarity of the
code. QEMU's codebase is glib based, by using glib functionality we
leverage developers' general familiarity with / knowledge of glib,
as opposed to custom QEMU approaches which need learning.

> - The macro def in glib depends on the glib version you use

If we need to depend on something that is newer than our min glib version,
then we add back compat definitino to 'include/glib-compat.h' for the older
version.

> - G_GNUC_FALLTHROUGH looks kind of abrasive to my eye, while  `fallthrough`
> blends in with other switch keywords like break.

My impression seeing this series, was exactly the opposite. I did not
like 'fallthrough' blending in with the rest of code, and also did not
like that it is a macro in lowercase, thus hiding that it is a macro

> - C23 standardises fallthrough. We might not ever support C23 but it's  good
> to be consistent with standards and other, larger projects (linux  kernel).

We're at least a decade away from being able to use anything from C23.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|