Re: [PATCH v2 2/6] iio: light: Add gain-time-scale helpers
From: Matti Vaittinen
Date: Mon Mar 13 2023 - 07:32:31 EST
On 3/6/23 13:13, Andy Shevchenko wrote:
On Fri, Mar 03, 2023 at 07:54:22AM +0000, Vaittinen, Matti wrote:
On 3/2/23 17:05, Andy Shevchenko wrote:
On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:
...
+{
+ int tmp = 1;
+
+ if (scale > max || !scale)
+ return -EINVAL;
+
+ if (U64_MAX - max < scale) {
+ /* Risk of overflow */
+ if (max - scale < scale)
+ return 1;
+ while (max - scale > scale * (u64) tmp)
Space is not required after casting.
+ tmp++;
+
+ return tmp + 1;
Wondering why you can't simplify this to
max -= scale;
tmp++;
Sorry, but I don't follow. Can you please show me the complete
suggestion? Exactly what should be replaced by the:
I don't understand. Do you see the blank lines I specifically added to show
the piece of the code I'm commenting on?
I saw. I, however, didn't see the point so I was thinking I
misunderstood what you wanted to replace.
For your convenience, the while loop and return statement may be replaced with
two simple assignments.
> max -= scale;
> tmp++;
Ah, thanks. Yes. I think that would yield the same result. Not sure if
it makes the logic more or less obvious, but it would kill one return
path which indeed (in my opinion) could make it look prettier.
That would require a local variable or dropping the const from max
though. Not sure it's worth that though.
...
+ for (i = gts->num_itime - 2; i >= 0; i--)
Yeah, if you put this into temporary, like
i = gts->num_itime - 1;
this becomes
while (i--) {
I prefer for(). It looks clearer to me...
It has too many characters to parse. That's why I prefer while.
Are we going to have principle disagreement on this one?
I think we have disagreed whether to prefer while() or for() also in the
past. I am reluctant to switch for()s to while()s unless there are other
obvious benefits. Especially I dislike the change if it involves -- or
++ in the condition. They tend to make it much more difficult (for me)
to see what the value is when comparing and what the value is after
exiting the loop. Yet, in this particular case I saw the value of
re-using the temporary value in memcpy() as you suggest below.
> Note, you may re-use that i (maybe renamed to something better in the
memcpy()
> above as well).
...but this makes sense so Ok.
...
+ for (i = 0; !ret && i < gts->num_avail_all_scales; i++)
Much easier to read if you move this...
+ ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+ >s->avail_all_scales_table[i * 2],
+ >s->avail_all_scales_table[i * 2 + 1]);
...here as
if (ret)
break;
I think the !ret in loop condition is obvious. Adding break and brackets
would not improve this.
It moves it to the regular pattern. Yours is not so distributed in the kernel.
I believe we can find examples of both patterns in kernel. I don't think
the "many people use different pattern" is a great reason to add break +
brackets which (in my eyes) give no additional value to code I am
planning to keep reading also in the future...
...
+ kfree(all_gains);
+ if (ret && gts->avail_all_scales_table)
+ kfree(gts->avail_all_scales_table);
+
+ return ret;
But Wouldn't be better to use goto labels?
I don't see benefit in this case. Handling return value and goto would
require brackets. The current one is much simpler for me. I am just
considering dropping that 'else' which is not needed.
At least it would be consistent with the very same file style in another
function. So, why there do you goto, and not here where it makes sense
to me?
Talking about the iio_gts_build_avail_scale_table()?
Difference is that the iio_gts_build_avail_scale_table() has multiple
checks for "if this happens - error out". I think using goto has valid
use-cases there.
To tell the truth - in the past I preferred "arrow code" pattern where I
initialized return values to default errors and only entered deeper to
nested code when checks were successful - changing error to succes only
at the deepest nested level. That however seems to rub the wrong way
most of the kernel developers. Hence I am adapted to use gotos for error
handling when we have more than only a few failure points.
In any case, here we can just do with single if-else (and for) without
even having the brackets. This is not a deeply nesting complex construct.
...
+ while (i) {
Instead of doing standard
while (i--) {
+ /*
+ * It does not matter if i'th alloc was not succesfull as
+ * kfree(NULL) is safe.
+ */
You add this comment, ...
+ kfree(per_time_gains[i]);
+ kfree(per_time_scales[i]);
...an additional loop, ...
The comment is there to explain what I think you missed. We have two
arrays there. We don't know whether the allocation of first one was
successful so we try freeing both.
+
+ i--;
...and a line of code.
+ }
Why?
Because, as the comment says, it does not matter if allocation was
succesfull. kfree(NULL) is safe.
Wouldn't be better to avoid no-op kfree() by explicitly putting the call before
hands?
kfree(...[i]);
while (i--) {
...
}
?
Much better to understand than what you wrote. I have to read comment on top of
the code, with my proposal comment won't be needed.
True, I can add one kfree() before the loop. It still does not change
the fact that we don't know if allocating the per_time_scales[i] was
succesful - unless we also add another label there. Well, it is a few
more LOC but avoids the kfree() and the comment so not really unbearable
trade :)
...
+ for (i = gts->num_itime - 1; i >= 0; i--) {
i = gts->num_itime;
while (i--) {
I prefer for() when initializing a variable is needed. Furthermore,
having var-- or --var in a condition is less clear.
while (x--)
_is_ a pattern for cleaning up something. Your for-loop has a lot of additional
(unnecessary) code lines that makes it harder to understand (by the people who
are not familiar with the pattern).
I do strongly disagree with you regarding
while (x--)
being simpler to understand than respective for. -- in a condition is
terrible - and I rarely use it.
I know I used it in v3 for the
while (time_idx--) {
and I am regretting that already.
+ }
...
+void iio_gts_purge_avail_time_table(struct iio_gts *gts)
+{
+ if (gts->num_avail_time_tables) {
if (!...)
return;
Does not improve this as we only have just one small conditional block
of code which we either execute or not. It is clear at a glance. Would
make sense if we had longer function.
I believe it makes sense to have like this even for this long function(s).
So, it seems like we do disagree with this too then. To me it is just
obvious here to check if we have tables to free and free them.
+ kfree(gts->avail_time_tables);
+ gts->avail_time_tables = NULL;
+ gts->num_avail_time_tables = 0;
+ }
+}
...
+ if (!diff) {
Why not positive conditional?
Because !diff is a special condition and we check explicitly for it.
And how my suggestion makes it different?
In example you gave we would be checking if the value is anything else
but the specific value we are checking for. It is counter intuitive.
(Note, it's easy to miss the ! in the conditionals, that's why positive ones
are preferable.)
Thank you for explaining me the rationale behind the "positive checks".
I didn't know missing '!' was seen as a thing.
I still don't think being afraid of missing '!' is a good reason to
switch to counter intuitive checks. A check "if (!foo)" is a pattern
in-kernel if anything and in my opinion people really should be aware of it.
(I would much more say that having a constant value on left side of a
"equality" check is beneficial as people do really occasionally miss one
'=' when meaning '=='. Still, this is not strong enough reason to make
counter-intuitive checks. In my books 'avoiding negative checks' is much
less of a reason as people (in my experience) do not really miss the '!'.)
Best Regards
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~