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

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

[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

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

[Peter Rosin]

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

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

[Peter Rosin]

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

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

[Jonathan Cameron]

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

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

[Gustavo A. R. Silva]

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

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

[Peter Rosin]

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

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

[Peter Rosin]

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=

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

[Jonathan Cameron]

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

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

[Gustavo A. R. Silva]

On 10/14/18 7:01 PM, Jonathan Cameron wrote:

> 
> Done the first of the above...
> 

Thank you, Jonathan.
--
Gustavo

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

[Peter Rosin]

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

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

[Peter Rosin]

T24gMjAxOC0xMC0xNiAxMzowMSwgR3VzdGF2byBBLiBSLiBTaWx2YSB3cm90ZToNCj4gT24gMTAv
MTQvMTggNzowMSBQTSwgSm9uYXRoYW4gQ2FtZXJvbiB3cm90ZToNCj4+IERvbmUgdGhlIGZpcnN0
IG9mIHRoZSBhYm92ZS4uLg0KPiANCj4gVGhhbmsgeW91LCBKb25hdGhhbi4NCg0KSW5kZWVkLiBJ
IG1lYW50IHRvIHJlc3BvbmQgZWFybGllciwgYnV0IHRoZW4gZm9yZ290Li4uIFRoYW5rIHlvdSEN
Cg0KQ2hlZXJzLA0KUGV0ZXINCg0K