Re: [Outreachy kernel] Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file

[Karolina Drobnik <karolinadrobnik@gmail.com>]

On Tue, 2021-10-19 at 05:26 -0700, Joe Perches wrote:
> What I suggested is not a patch it's just an example.

Sure, I understand. In this case, I'll take some inspiration from
your example and break down the changes into smaller chunks, thank you.

> There's quite a lot of code in that driver that _could_
> be updated/refined/refactored (none of which _I_ will submit),
> but it's up to you do whatever _you_ want.

Indeed there is, I'm trying to tackle one thing at a time. I thought
I could fix a couple of line length warnings in an easy way but I was
wrong.

Ok, I'll come back to CamelCase squashing and removing the Hungarian
notation before working on this refactor. I think this is a good
candidate for a patchset.

On Tue, 2021-10-19 at 15:07 +0200, Fabio M. De Francesco wrote:
> Hi Karolina,

Hi Fabio,
Thank you for describing everything in such detail, really appreciate
it.

> No, there is no problem in using a[i - 1]. Personally I prefer the
> former when 1 <= index <= ARRAY_SIZE(a). 

I see, thanks for explaining this.

> If you code "index = index -1;" or 
> "index--;" (that is the same) and then you use 'index' many lines
> below that decrement in "a[index]" it may be not immediately clear
> that you are not indexing past the end of the array.

That's what I thought as well.

> I prefer to state it again: if you choose to do such kind of works,
> be careful to split self-contained patches in a series and explain
> each change you make and why you make it.
> Each patch must do only one logical change.

Will definitely do so, thank you.

> Each patch of a series must be self-contained also in the sense that
> it must build without introducing errors or warnings at any point:
> for instance, five patches => five clean builds.

Makes sense, will keep that in mind. Also, I think it would be good to
mention it on the FPT page. I can suggest adding such comment in later
on.


Thanks,
Karolina