* [PATCH] iio: dpot-dac: mark expected switch fall-through @ 2018-10-08 17:35 Gustavo A. R. Silva 2018-10-08 20:42 ` Peter Rosin 0 siblings, 1 reply; 11+ messages in thread From: Gustavo A. R. Silva @ 2018-10-08 17:35 UTC (permalink / raw) To: Peter Rosin, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler Cc: linux-iio, linux-kernel, Gustavo A. R. Silva 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(-) 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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: dpot-dac: mark expected switch fall-through 2018-10-08 17:35 [PATCH] iio: dpot-dac: mark expected switch fall-through Gustavo A. R. Silva @ 2018-10-08 20:42 ` Peter Rosin 0 siblings, 0 replies; 11+ messages in thread From: Peter Rosin @ 2018-10-08 20:42 UTC (permalink / raw) To: Gustavo A. R. Silva, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler Cc: linux-iio, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: dpot-dac: mark expected switch fall-through @ 2018-10-08 20:42 ` Peter Rosin 0 siblings, 0 replies; 11+ messages in thread From: Peter Rosin @ 2018-10-08 20:42 UTC (permalink / raw) To: Gustavo A. R. Silva, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler Cc: linux-iio, linux-kernel T24gMjAxOC0xMC0wOCAxOTozNSwgR3VzdGF2byBBLiBSLiBTaWx2YSB3cm90ZToNCj4gSW4gcHJl cGFyYXRpb24gdG8gZW5hYmxpbmcgLVdpbXBsaWNpdC1mYWxsdGhyb3VnaCwgbWFyayBzd2l0Y2gg Y2FzZXMNCj4gd2hlcmUgd2UgYXJlIGV4cGVjdGluZyB0byBmYWxsIHRocm91Z2guDQoNClRoZSB3 YXkgSSBzZWUgaXQsIGl0IGlzIHByZXR0eSB3ZWxsIG1hcmtlZCB1cCBhcyBpcy4gU28sIHRoaXMg cGFyYWdyYXBoDQppcyBub3QgZGVzY3JpYmluZyB0aGUgY2hhbmdlLg0KDQo+IA0KPiBOb3RpY2Ug dGhhdCBpbiB0aGlzIHBhcnRpY3VsYXIgY2FzZSwgSSByZXBsYWNlZCAiLi4uYW5kIGZhbGwgdGhy b3VnaC4iDQo+IHdpdGggYSBwcm9wZXIgImZhbGwgdGhyb3VnaCIsIHdoaWNoIGlzIHdoYXQgR0ND IGlzIGV4cGVjdGluZyB0byBmaW5kLg0KDQpXaGF0IGlzIG5vdCAicHJvcGVyIiBhYm91dCB0aGUg ZXhpc3RpbmcgY29tbWVudD8gWWVzIHllcywgSSAqa25vdyogdGhhdA0KR0NDIGlzIG5vdCB2ZXJ5 IGludGVsbGlnZW50IGFib3V0IGl0IGFuZCByZXF1aXJlcyBoYW5kLWhvbGRpbmcsIGJ1dA0KYmxh bWluZyB0aGUgZXhpc3RpbmcgY29tbWVudCBmb3Igbm90ICpwcm9wZXJseSogbWFya2luZyBhbiBp bnRlbnRpb25hbA0KZmFsbCB0aHJvdWdoIGlzIC4uLiByaWNoLg0KDQo+IA0KPiBBZGRyZXNzZXMt Q292ZXJpdHktSUQ6IDE0NjI0MDggKCJNaXNzaW5nIGJyZWFrIGluIHN3aXRjaCIpDQo+IFNpZ25l ZC1vZmYtYnk6IEd1c3Rhdm8gQS4gUi4gU2lsdmEgPGd1c3Rhdm9AZW1iZWRkZWRvci5jb20+DQo+ IC0tLQ0KPiAgZHJpdmVycy9paW8vZGFjL2Rwb3QtZGFjLmMgfCAyICstDQo+ICAxIGZpbGUgY2hh bmdlZCwgMSBpbnNlcnRpb24oKyksIDEgZGVsZXRpb24oLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9k cml2ZXJzL2lpby9kYWMvZHBvdC1kYWMuYyBiL2RyaXZlcnMvaWlvL2RhYy9kcG90LWRhYy5jDQo+ IGluZGV4IGE3OTFkMGEuLmUzNTM5NDYgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMvaWlvL2RhYy9k cG90LWRhYy5jDQo+ICsrKyBiL2RyaXZlcnMvaWlvL2RhYy9kcG90LWRhYy5jDQo+IEBAIC03OCw3 ICs3OCw3IEBAIHN0YXRpYyBpbnQgZHBvdF9kYWNfcmVhZF9yYXcoc3RydWN0IGlpb19kZXYgKmlu ZGlvX2RldiwNCg0KQWRkaW5nIHNvbWUgbW9yZSBjb250ZXh0IGhlcmUuDQoNCgkJY2FzZSBJSU9f VkFMX0lOVDoNCgkJCS8qDQoJCQkgKiBDb252ZXJ0IGludGVnZXIgc2NhbGUgdG8gZnJhY3Rpb25h bCBzY2FsZSBieQ0KCQkJICogc2V0dGluZyB0aGUgZGVub21pbmF0b3IgKHZhbDIpIHRvIG9uZS4u Lg0KPiAgCQkJICovDQo+ICAJCQkqdmFsMiA9IDE7DQo+ICAJCQlyZXQgPSBJSU9fVkFMX0ZSQUNU SU9OQUw7DQo+IC0JCQkvKiAuLi5hbmQgZmFsbCB0aHJvdWdoLiAqLw0KPiArCQkJLyogZmFsbCB0 aHJvdWdoICovDQo+ICAJCWNhc2UgSUlPX1ZBTF9GUkFDVElPTkFMOg0KPiAgCQkJKnZhbCAqPSBy ZWd1bGF0b3JfZ2V0X3ZvbHRhZ2UoZGFjLT52cmVmKSAvIDEwMDA7DQo+ICAJCQkqdmFsMiAqPSBk YWMtPm1heF9vaG1zOw0KPiANCg0KQ29uc2lkZXJpbmcgdGhlIGFib3ZlIGFkZGVkIGNvbnRleHQs IEkgaGF2ZSB0byBzYXkgdGhhdCB0aGlzIG1pbmRsZXNzDQpjaGFuZ2UgaXMgbm90IGFuIGltcHJv dmVtZW50LCBhcyB5b3UgaGF2ZSBqdXN0IGRlc3Ryb3llZCB0aGUgY29udGludWVkDQpzZW50ZW5j ZSBmcm9tIHRoZSBwcmV2aW91cyBjb21tZW50LiBZb3UgbXVzdCBoYXZlIG5vdGljZWQgdGhhdCB0 aGlzDQp3YXMgdGhlIGVuZCBvZiBhIGNvbnRpbnVlZCBzZW50ZW5jZSwgYXMgeW91IGV2ZW4gcXVv dGVkIGl0IGluIHRoZSBjb21taXQNCm1lc3NhZ2UuIFRoZSBiaWcgcXVlc3Rpb24gaXMgd2h5IHlv dSBkaWQgbm90IHN0b3AgdG8gdGhpbmsgYW5kIGNvbnNpZGVyDQp0aGUgY29udGV4dD8NCg0KWWVz LCBJJ20gYW5ub3llZCBieSBtaW5kbGVzcyBjaGFuZ2VzLiBFc3BlY2lhbGx5IG1pbmRsZXNzIGNo YW5nZXMgYWltZWQNCmF0IGltcHJvdmluZyByZWFkYWJpbGl0eSB3aGlsZSBpbiBmYWN0IG1ha2lu ZyB0aGluZ3MgbGVzcyByZWFkYWJsZS4NCg0KVEw7RFIsIGlmIHlvdSBhcmUgZGVzcGVyYXRlIHRv IGZpeCAidGhlIHByb2JsZW0iIHdpdGggdGhpcyBmYWxsIHRocm91Z2gNCmNvbW1lbnQsIHBsZWFz ZSBkbyBzbyBpbiBhIHdheSB0aGF0IHByZXNlcnZlcyBvdmVyYWxsIHJlYWRhYmlsaXR5LiBBbmQN Cml0IHdvdWxkIGJlIG5pY2UgdG8gbm90IGJsYW1lIHRoZSBleGlzdGluZyBjb2RlIGZvciBicmFp biBkYW1hZ2UgaW4gR0NDDQphbmQgdmFyaW91cyBvdGhlciBzdGF0aWMgYW5hbHl6ZXJzLg0KDQpD aGVlcnMsDQpQZXRlcg0K ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: dpot-dac: mark expected switch fall-through 2018-10-08 20:42 ` Peter Rosin (?) @ 2018-10-13 12:38 ` Jonathan Cameron 2018-10-13 13:08 ` Gustavo A. R. Silva 2018-10-13 15:14 ` Peter Rosin -1 siblings, 2 replies; 11+ messages in thread From: Jonathan Cameron @ 2018-10-13 12:38 UTC (permalink / raw) To: Peter Rosin Cc: Gustavo A. R. Silva, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: dpot-dac: mark expected switch fall-through 2018-10-13 12:38 ` Jonathan Cameron @ 2018-10-13 13:08 ` Gustavo A. R. Silva 2018-10-13 15:14 ` Peter Rosin 1 sibling, 0 replies; 11+ messages in thread From: Gustavo A. R. Silva @ 2018-10-13 13:08 UTC (permalink / raw) To: Jonathan Cameron, Peter Rosin Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: dpot-dac: mark expected switch fall-through 2018-10-13 12:38 ` Jonathan Cameron @ 2018-10-13 15:14 ` Peter Rosin 2018-10-13 15:14 ` Peter Rosin 1 sibling, 0 replies; 11+ messages in thread From: Peter Rosin @ 2018-10-13 15:14 UTC (permalink / raw) To: Jonathan Cameron Cc: Gustavo A. R. Silva, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: dpot-dac: mark expected switch fall-through @ 2018-10-13 15:14 ` Peter Rosin 0 siblings, 0 replies; 11+ messages in thread From: Peter Rosin @ 2018-10-13 15:14 UTC (permalink / raw) To: Jonathan Cameron Cc: Gustavo A. R. Silva, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel T24gMjAxOC0xMC0xMyAxNDozOCwgSm9uYXRoYW4gQ2FtZXJvbiB3cm90ZToNCj4gT24gTW9uLCA4 IE9jdCAyMDE4IDIwOjQyOjQxICswMDAwDQo+IFBldGVyIFJvc2luIDxwZWRhQGF4ZW50aWEuc2U+ IHdyb3RlOg0KPiANCj4+IE9uIDIwMTgtMTAtMDggMTk6MzUsIEd1c3Rhdm8gQS4gUi4gU2lsdmEg d3JvdGU6DQo+Pj4gSW4gcHJlcGFyYXRpb24gdG8gZW5hYmxpbmcgLVdpbXBsaWNpdC1mYWxsdGhy b3VnaCwgbWFyayBzd2l0Y2ggY2FzZXMNCj4+PiB3aGVyZSB3ZSBhcmUgZXhwZWN0aW5nIHRvIGZh bGwgdGhyb3VnaC4gIA0KPj4NCj4+IFRoZSB3YXkgSSBzZWUgaXQsIGl0IGlzIHByZXR0eSB3ZWxs IG1hcmtlZCB1cCBhcyBpcy4gU28sIHRoaXMgcGFyYWdyYXBoDQo+PiBpcyBub3QgZGVzY3JpYmlu ZyB0aGUgY2hhbmdlLg0KPj4NCj4+Pg0KPj4+IE5vdGljZSB0aGF0IGluIHRoaXMgcGFydGljdWxh ciBjYXNlLCBJIHJlcGxhY2VkICIuLi5hbmQgZmFsbCB0aHJvdWdoLiINCj4+PiB3aXRoIGEgcHJv cGVyICJmYWxsIHRocm91Z2giLCB3aGljaCBpcyB3aGF0IEdDQyBpcyBleHBlY3RpbmcgdG8gZmlu ZC4gIA0KPj4NCj4+IFdoYXQgaXMgbm90ICJwcm9wZXIiIGFib3V0IHRoZSBleGlzdGluZyBjb21t ZW50PyBZZXMgeWVzLCBJICprbm93KiB0aGF0DQo+PiBHQ0MgaXMgbm90IHZlcnkgaW50ZWxsaWdl bnQgYWJvdXQgaXQgYW5kIHJlcXVpcmVzIGhhbmQtaG9sZGluZywgYnV0DQo+PiBibGFtaW5nIHRo ZSBleGlzdGluZyBjb21tZW50IGZvciBub3QgKnByb3Blcmx5KiBtYXJraW5nIGFuIGludGVudGlv bmFsDQo+PiBmYWxsIHRocm91Z2ggaXMgLi4uIHJpY2guDQo+Pg0KPj4+DQo+Pj4gQWRkcmVzc2Vz LUNvdmVyaXR5LUlEOiAxNDYyNDA4ICgiTWlzc2luZyBicmVhayBpbiBzd2l0Y2giKQ0KPj4+IFNp Z25lZC1vZmYtYnk6IEd1c3Rhdm8gQS4gUi4gU2lsdmEgPGd1c3Rhdm9AZW1iZWRkZWRvci5jb20+ DQo+Pj4gLS0tDQo+Pj4gIGRyaXZlcnMvaWlvL2RhYy9kcG90LWRhYy5jIHwgMiArLQ0KPj4+ICAx IGZpbGUgY2hhbmdlZCwgMSBpbnNlcnRpb24oKyksIDEgZGVsZXRpb24oLSkNCj4+Pg0KPj4+IGRp ZmYgLS1naXQgYS9kcml2ZXJzL2lpby9kYWMvZHBvdC1kYWMuYyBiL2RyaXZlcnMvaWlvL2RhYy9k cG90LWRhYy5jDQo+Pj4gaW5kZXggYTc5MWQwYS4uZTM1Mzk0NiAxMDA2NDQNCj4+PiAtLS0gYS9k cml2ZXJzL2lpby9kYWMvZHBvdC1kYWMuYw0KPj4+ICsrKyBiL2RyaXZlcnMvaWlvL2RhYy9kcG90 LWRhYy5jDQo+Pj4gQEAgLTc4LDcgKzc4LDcgQEAgc3RhdGljIGludCBkcG90X2RhY19yZWFkX3Jh dyhzdHJ1Y3QgaWlvX2RldiAqaW5kaW9fZGV2LCAgDQo+Pg0KPj4gQWRkaW5nIHNvbWUgbW9yZSBj b250ZXh0IGhlcmUuDQo+Pg0KPj4gCQljYXNlIElJT19WQUxfSU5UOg0KPj4gCQkJLyoNCj4+IAkJ CSAqIENvbnZlcnQgaW50ZWdlciBzY2FsZSB0byBmcmFjdGlvbmFsIHNjYWxlIGJ5DQo+PiAJCQkg KiBzZXR0aW5nIHRoZSBkZW5vbWluYXRvciAodmFsMikgdG8gb25lLi4uDQo+Pj4gIAkJCSAqLw0K Pj4+ICAJCQkqdmFsMiA9IDE7DQo+Pj4gIAkJCXJldCA9IElJT19WQUxfRlJBQ1RJT05BTDsNCj4+ PiAtCQkJLyogLi4uYW5kIGZhbGwgdGhyb3VnaC4gKi8NCj4+PiArCQkJLyogZmFsbCB0aHJvdWdo ICovDQo+Pj4gIAkJY2FzZSBJSU9fVkFMX0ZSQUNUSU9OQUw6DQo+Pj4gIAkJCSp2YWwgKj0gcmVn dWxhdG9yX2dldF92b2x0YWdlKGRhYy0+dnJlZikgLyAxMDAwOw0KPj4+ICAJCQkqdmFsMiAqPSBk YWMtPm1heF9vaG1zOw0KPj4+ICAgDQo+Pg0KPj4gQ29uc2lkZXJpbmcgdGhlIGFib3ZlIGFkZGVk IGNvbnRleHQsIEkgaGF2ZSB0byBzYXkgdGhhdCB0aGlzIG1pbmRsZXNzDQo+PiBjaGFuZ2UgaXMg bm90IGFuIGltcHJvdmVtZW50LCBhcyB5b3UgaGF2ZSBqdXN0IGRlc3Ryb3llZCB0aGUgY29udGlu dWVkDQo+PiBzZW50ZW5jZSBmcm9tIHRoZSBwcmV2aW91cyBjb21tZW50LiBZb3UgbXVzdCBoYXZl IG5vdGljZWQgdGhhdCB0aGlzDQo+PiB3YXMgdGhlIGVuZCBvZiBhIGNvbnRpbnVlZCBzZW50ZW5j ZSwgYXMgeW91IGV2ZW4gcXVvdGVkIGl0IGluIHRoZSBjb21taXQNCj4+IG1lc3NhZ2UuIFRoZSBi aWcgcXVlc3Rpb24gaXMgd2h5IHlvdSBkaWQgbm90IHN0b3AgdG8gdGhpbmsgYW5kIGNvbnNpZGVy DQo+PiB0aGUgY29udGV4dD8NCj4+DQo+PiBZZXMsIEknbSBhbm5veWVkIGJ5IG1pbmRsZXNzIGNo YW5nZXMuIEVzcGVjaWFsbHkgbWluZGxlc3MgY2hhbmdlcyBhaW1lZA0KPj4gYXQgaW1wcm92aW5n IHJlYWRhYmlsaXR5IHdoaWxlIGluIGZhY3QgbWFraW5nIHRoaW5ncyBsZXNzIHJlYWRhYmxlLg0K Pj4NCj4+IFRMO0RSLCBpZiB5b3UgYXJlIGRlc3BlcmF0ZSB0byBmaXggInRoZSBwcm9ibGVtIiB3 aXRoIHRoaXMgZmFsbCB0aHJvdWdoDQo+PiBjb21tZW50LCBwbGVhc2UgZG8gc28gaW4gYSB3YXkg dGhhdCBwcmVzZXJ2ZXMgb3ZlcmFsbCByZWFkYWJpbGl0eS4gQW5kDQo+PiBpdCB3b3VsZCBiZSBu aWNlIHRvIG5vdCBibGFtZSB0aGUgZXhpc3RpbmcgY29kZSBmb3IgYnJhaW4gZGFtYWdlIGluIEdD Qw0KPj4gYW5kIHZhcmlvdXMgb3RoZXIgc3RhdGljIGFuYWx5emVycy4NCj4+DQo+PiBDaGVlcnMs DQo+PiBQZXRlcg0KPiBJIGFncmVlIHdpdGggeW91IGluIHByaW5jaXBsZSBQZXRlciBhbmQgaGF2 ZSB0d2Vha2VkIHRoZSBwYXRjaCBkZXNjcmlwdGlvbg0KPiB0byBtYWtlIGl0IGNsZWFyZXIgdGhh dCB3ZSBhcmUgZG9pbmcgdGhpcyB0byBtYWtlIEdDQyBzdGF0aWMgYW5hbHlzaXMgbW9yZQ0KPiBo ZWxwZnVsIChzdXBwcmVzc2luZyBhIGZhbHNlIHdhcm5pbmcgaXMgYSB3b3J0aHdoaWxlIGlmIHlv dSBhcmUgZGVhbGluZyB3aXRoDQo+IGxvdHMgb2YgdGhlbSkuDQo+IA0KPiBIb3dldmVyLCBuaWNl IHRob3VnaCBpdCBpcyB0byBoYXZlIGVsZWdhbnQgY29tbWVudCBzdHJ1Y3R1cmUgSSB0aGluayB3 ZQ0KPiBzaG91bGQgc3RpbGwgaGF2ZSB0aGlzIHBhdGNoIGluIHBsYWNlLiAgVGhpcyBlZmZvcnQg dG8gJ2ZpeCcgdGhlc2UNCj4gd2FybmluZ3MgaGFzIGFscmVhZHkgaWRlbnRpZmllZCBhIGZldyBw bGFjZXMgd2hlcmUgaXQgd2FzIHdyb25nIHNvDQo+IEknbSBrZWVuIHRvIHNlZSBpdCBhcHBsaWVk IGJ5IGRlZmF1bHQgZXZlbiBpZiBpdCBpc24ndCBwZXJmZWN0Lg0KDQpJIHN0aWxsIG9iamVjdC4g SXQgd291bGQgaGF2ZSBiZWVuIHNvIGRhbW4gZWFzeSBhbmQgaXQgZG9lcyBub3QgdGFrZSBhIHdo b2xlDQpsb3Qgb2YgaW1hZ2luYXRpb24gdG8gcXVpZXQgZG93biBHQ0Mgd2hpbGUga2VlcGluZyB0 aGUgY29tbWVudHMgcmVhZGFibGUuIEp1c3QNCm1vdmUgdGhlICJhbmQiIHRvIHRoZSBwcmV2aW91 cyBjb21tZW50LCBsaWtlIHRoaXMuDQoNCgkJY2FzZSBJSU9fVkFMX0lOVDoNCgkJCS8qDQoJCQkg KiBDb252ZXJ0IGludGVnZXIgc2NhbGUgdG8gZnJhY3Rpb25hbCBzY2FsZSBieQ0KCQkJICogc2V0 dGluZyB0aGUgZGVub21pbmF0b3IgKHZhbDIpIHRvIG9uZSwgYW5kLi4uDQoJCQkgKi8NCgkJCSp2 YWwyID0gMTsNCgkJCXJldCA9IElJT19WQUxfRlJBQ1RJT05BTDsNCgkJCS8qIGZhbGwgdGhyb3Vn aCAqLw0KCQljYXNlIElJT19WQUxfRlJBQ1RJT05BTDoNCg0KT3IgYWRkIGEgc2VudGVuY2UsIGxp a2UgdGhpcyAod2hpY2ggaXMgYSBiaXQgbW9yZSBmdW4gSU1PKQ0KDQoJCWNhc2UgSUlPX1ZBTF9J TlQ6DQoJCQkvKg0KCQkJICogQ29udmVydCBpbnRlZ2VyIHNjYWxlIHRvIGZyYWN0aW9uYWwgc2Nh bGUgYnkNCgkJCSAqIHNldHRpbmcgdGhlIGRlbm9taW5hdG9yICh2YWwyKSB0byBvbmUuLi4NCgkJ CSAqLw0KCQkJKnZhbDIgPSAxOw0KCQkJcmV0ID0gSUlPX1ZBTF9GUkFDVElPTkFMOw0KCQkJLyog Li4uYW5kIGZhbGwgdGhyb3VnaC4gU2F5IGl0IGFnYWluIGZvciBHQ0MuICovDQoJCQkvKiBmYWxs IHRocm91Z2ggKi8NCgkJY2FzZSBJSU9fVkFMX0ZSQUNUSU9OQUw6DQoNCkNoZWVycywNClBldGVy DQo= ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: dpot-dac: mark expected switch fall-through 2018-10-13 15:14 ` Peter Rosin (?) @ 2018-10-14 17:01 ` Jonathan Cameron 2018-10-16 11:01 ` Gustavo A. R. Silva -1 siblings, 1 reply; 11+ messages in thread From: Jonathan Cameron @ 2018-10-14 17:01 UTC (permalink / raw) To: Peter Rosin Cc: Gustavo A. R. Silva, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: dpot-dac: mark expected switch fall-through 2018-10-14 17:01 ` Jonathan Cameron @ 2018-10-16 11:01 ` Gustavo A. R. Silva 2018-10-17 6:55 ` Peter Rosin 0 siblings, 1 reply; 11+ messages in thread From: Gustavo A. R. Silva @ 2018-10-16 11:01 UTC (permalink / raw) To: Jonathan Cameron, Peter Rosin Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel On 10/14/18 7:01 PM, Jonathan Cameron wrote: > > Done the first of the above... > Thank you, Jonathan. -- Gustavo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: dpot-dac: mark expected switch fall-through 2018-10-16 11:01 ` Gustavo A. R. Silva @ 2018-10-17 6:55 ` Peter Rosin 0 siblings, 0 replies; 11+ messages in thread From: Peter Rosin @ 2018-10-17 6:55 UTC (permalink / raw) To: Gustavo A. R. Silva, Jonathan Cameron Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: dpot-dac: mark expected switch fall-through @ 2018-10-17 6:55 ` Peter Rosin 0 siblings, 0 replies; 11+ messages in thread From: Peter Rosin @ 2018-10-17 6:55 UTC (permalink / raw) To: Gustavo A. R. Silva, Jonathan Cameron Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel T24gMjAxOC0xMC0xNiAxMzowMSwgR3VzdGF2byBBLiBSLiBTaWx2YSB3cm90ZToNCj4gT24gMTAv MTQvMTggNzowMSBQTSwgSm9uYXRoYW4gQ2FtZXJvbiB3cm90ZToNCj4+IERvbmUgdGhlIGZpcnN0 IG9mIHRoZSBhYm92ZS4uLg0KPiANCj4gVGhhbmsgeW91LCBKb25hdGhhbi4NCg0KSW5kZWVkLiBJ IG1lYW50IHRvIHJlc3BvbmQgZWFybGllciwgYnV0IHRoZW4gZm9yZ290Li4uIFRoYW5rIHlvdSEN Cg0KQ2hlZXJzLA0KUGV0ZXINCg0K ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-10-17 6:55 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-08 17:35 [PATCH] iio: dpot-dac: mark expected switch fall-through Gustavo A. R. Silva 2018-10-08 20:42 ` Peter Rosin 2018-10-08 20:42 ` Peter Rosin 2018-10-13 12:38 ` Jonathan Cameron 2018-10-13 13:08 ` Gustavo A. R. Silva 2018-10-13 15:14 ` Peter Rosin 2018-10-13 15:14 ` Peter Rosin 2018-10-14 17:01 ` Jonathan Cameron 2018-10-16 11:01 ` Gustavo A. R. Silva 2018-10-17 6:55 ` Peter Rosin 2018-10-17 6:55 ` Peter Rosin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.