| Message ID | 20181008173528.GA31787@embeddedor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: dpot-dac: mark expected switch fall-through | expand |
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
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
On 10/13/18 2:38 PM, Jonathan Cameron wrote: > 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. > Thanks, Jonathan. Below are some examples of cases in which the fall-through warning turned out to be an actual bug: commit c24bfa8f21b59283580043dada19a6e943b6e426 commit ad0eaee6195db1db1749dd46b9e6f4466793d178 commit 9ba8376ce1e2cbf4ce44f7e4bee1d0648e10d594 commit dc586a60a11d0260308db1bebe788ad8973e2729 commit a8e9b186f153a44690ad0363a56716e7077ad28c commit 4e57562b4846e42cd1c2e556f0ece18c1154e116 commit 7c92e5fbf4dac0dd4dd41a0383adc54f16f403e2 commit c5b974bee9d2ceae4c441ae5a01e498c2674e100 commit 2c930e3d0aed1505e86e0928d323df5027817740 commit 882518debc8487147d618d5f26f4bb0bea1cc05b commit f745e9cc7e40c4570ab5e8d5ef32bfaa6e8ced46 commit 5dc874252faa818426480a7c00fa05738fe05402 commit 4a00aa057759d713e1296ecbc614fa560d569977 commit 6d3f06a0042ebd59a5e9d4ba6e8a85596901e140 commit 827d240a232d27cc12e9657d012f2e5ba953e98a commit a28b259b43914b04746184cec318c67bded7234c commit 9e7b319e1d1e6cba41ae96f791789a7806b29584 commit d393be3ed0bebb30a4666d7f5ed4486cd6b31716 commit 680682d4d537565e2c358483e1feeca30a8cf3d4 commit 06af9b0f4949b85b20107e6d75f5eba15111d220 So, yeah. This effort is worth it. Thanks -- Gustavo
On 2018-10-13 14:38, Jonathan Cameron wrote: > 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. 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
On Sat, 13 Oct 2018 15:14:34 +0000 Peter Rosin <peda@axentia.se> wrote: > On 2018-10-13 14:38, Jonathan Cameron wrote: > > 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. > > 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
On 10/14/18 7:01 PM, Jonathan Cameron wrote: > > Done the first of the above... > Thank you, Jonathan. -- Gustavo
On 2018-10-16 13:01, Gustavo A. R. Silva wrote: > On 10/14/18 7:01 PM, Jonathan Cameron wrote: >> Done the first of the above... > > Thank you, Jonathan. Indeed. I meant to respond earlier, but then forgot... Thank you! Cheers, Peter
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, */ *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;
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Notice that in this particular case, I replaced "...and fall through." with a proper "fall through", which is what GCC is expecting to find. 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(-)