On Fri, Mar 05, 2021 at 10:58:17AM -0600, Edmundo Carmona Antoranz wrote:
> On Fri, Mar 5, 2021 at 2:59 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > - if (sec_len > 0 && sec_len <= len) {
> > + if (sec_len > 0 &&
> > + sec_len <= len &&
> > + sec_len <= 32) {
>
> I wonder if this could be reduced to (sec_len > 0 && sec_len <=
> min(len, 32)) from a stylistic POV?
I kind of prefer it the way I wrote it. I prefer conditions split
apart and done ploddingly, one at a time... You'll notice how I could
have written it like:
if (sec_len > 0 && sec_len <= len &&
sec_len <= 32) {
But I really like my conditions to be spelled out so the "sec_len" is
perfectly aligned in each part of the condition. Your way would be to
combine two conditions into one part of a line and seems sneaky.
>
> First attempt at something kernel related so I know there's plenty of
> things to learn (including patterns for problems like this and
> etiquette).
It's good that you're reviewing code... We try to be predictable though
and no one would have predicted your response. Ideally patch review
should be like, "Ugh! Why didn't I think of that? Of course, we should
propagate the error code." Or "Oh, I didn't know checkpatch warns about
that."
The truth is that I don't always agree with all of Greg's reviews. He
is more strict than I am about breaking up patches into multiple things.
(It's a tricky line to define for me). But I can always predict what
Greg will say in a review so that saves time when I know which patches
he will accept and which he won't.
regards,
dan carpenter
