Messages in this thread | | | Subject | Re: [Outreachy kernel] Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file | From | Karolina Drobnik <> | Date | Tue, 19 Oct 2021 16:08:25 +0100 |
| |
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
| |