Re: [PATCH] iio: dpot-dac: mark expected switch fall-through

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 8 Oct 2018 20:42:41 +0000
Peter Rosin <peda@axentia.se> wrote:

> On 2018-10-08 19:35, Gustavo A. R. Silva wrote:
> > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > where we are expecting to fall through.  
> 
> The way I see it, it is pretty well marked up as is. So, this paragraph
> is not describing the change.
> 
> > 
> > Notice that in this particular case, I replaced "...and fall through."
> > with a proper "fall through", which is what GCC is expecting to find.  
> 
> What is not "proper" about the existing comment? Yes yes, I *know* that
> GCC is not very intelligent about it and requires hand-holding, but
> blaming the existing comment for not *properly* marking an intentional
> fall through is ... rich.
> 
> > 
> > Addresses-Coverity-ID: 1462408 ("Missing break in switch")
> > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > ---
> >  drivers/iio/dac/dpot-dac.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/dac/dpot-dac.c b/drivers/iio/dac/dpot-dac.c
> > index a791d0a..e353946 100644
> > --- a/drivers/iio/dac/dpot-dac.c
> > +++ b/drivers/iio/dac/dpot-dac.c
> > @@ -78,7 +78,7 @@ static int dpot_dac_read_raw(struct iio_dev *indio_dev,  
> 
> Adding some more context here.
> 
> 		case IIO_VAL_INT:
> 			/*
> 			 * Convert integer scale to fractional scale by
> 			 * setting the denominator (val2) to one...
> >  			 */
> >  			*val2 = 1;
> >  			ret = IIO_VAL_FRACTIONAL;
> > -			/* ...and fall through. */
> > +			/* fall through */
> >  		case IIO_VAL_FRACTIONAL:
> >  			*val *= regulator_get_voltage(dac->vref) / 1000;
> >  			*val2 *= dac->max_ohms;
> >   
> 
> Considering the above added context, I have to say that this mindless
> change is not an improvement, as you have just destroyed the continued
> sentence from the previous comment. You must have noticed that this
> was the end of a continued sentence, as you even quoted it in the commit
> message. The big question is why you did not stop to think and consider
> the context?
> 
> Yes, I'm annoyed by mindless changes. Especially mindless changes aimed
> at improving readability while in fact making things less readable.
> 
> TL;DR, if you are desperate to fix "the problem" with this fall through
> comment, please do so in a way that preserves overall readability. And
> it would be nice to not blame the existing code for brain damage in GCC
> and various other static analyzers.
> 
> Cheers,
> Peter
I agree with you in principle Peter and have tweaked the patch description
to make it clearer that we are doing this to make GCC static analysis more
helpful (suppressing a false warning is a worthwhile if you are dealing with
lots of them).

However, nice though it is to have elegant comment structure I think we
should still have this patch in place.  This effort to 'fix' these
warnings has already identified a few places where it was wrong so
I'm keen to see it applied by default even if it isn't perfect.

Jonathan