On Sat, 13 Oct 2018 15:14:34 +0000 Peter Rosin <[email protected]> wrote:
> On 2018-10-13 14:38, Jonathan Cameron wrote: > > On Mon, 8 Oct 2018 20:42:41 +0000 > > Peter Rosin <[email protected]> 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 <[email protected]> > >>> --- > >>> 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. > > I still object. It would have been so damn easy and it does not take a whole > lot of imagination to quiet down GCC while keeping the comments readable. Just > move the "and" to the previous comment, like this. > > case IIO_VAL_INT: > /* > * Convert integer scale to fractional scale by > * setting the denominator (val2) to one, and... > */ > *val2 = 1; > ret = IIO_VAL_FRACTIONAL; > /* fall through */ > case IIO_VAL_FRACTIONAL: > > Or add a sentence, like this (which is a bit more fun IMO) > > 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. Say it again for GCC. */ > /* fall through */ > case IIO_VAL_FRACTIONAL: > > Cheers, > Peter Done the first of the above... Thanks, Jonathan
