Thread (31 messages) 31 messages, 4 authors, 2021-09-14

Re: [PATCH v4 15/18] staging: r8188eu: hal: Clean up usbctrl_vendorreq()

From: Fabio M. De Francesco <hidden>
Date: 2021-09-14 11:18:13
Also in: lkml

On Tuesday, September 14, 2021 11:24:05 AM CEST Dan Carpenter wrote:
On Mon, Sep 13, 2021 at 08:09:59PM +0200, Fabio M. De Francesco wrote:
quoted
Clean up usbctrl_vendorreq () in usb_ops_linux.c because some
of its code will be reused in this series. This cleanup is in
preparation for shortening the call chains of rtw_read{8,16,32}()
and rtw_write{8,16,32,N}(). More insights about the reasons why at
https://lore.kernel.org/lkml/5319192.FrU0QrjFp7@localhost.localdomain/ (local)
This commit message is quite bad.

This patch has nothing to do with reusing the code or shortening call
chains.
It has to do, in a certain sense. Let me explain please...

Some days ago, David Laight made the review of "Shorten calls chain of 
rtw_write8/16/32/n()" version 3. 

In that patch he noticed some lines of usb_read() that I had created with the 
help of reusing some lines of the code of usbctrl_vendorreq() that is deleted 
in the same patch. 

He thought that they were clean-ups and renames and so he suggested to make 
those "clean-ups" in a separate patch.

However they were _not_ renames or other clean-ups, because usb_read() was 
not touched in that patch and, above all, it was a new function. 

I am sure that when I write new functions I can use whatever name of 
variables I like, even if people may think I'm renaming the variables that 
were in a old function that now is deleted. Am I not permitted?

However, because I also think that readability of the diffs matters, I 
decided to do some clean-up of the code I'm about to reuse in the new 
functions. It improves readability of the above-mentioned patch that is also 
the 18/18 of this series.

That is the reason why I'm cleaning up a function that is going to be deleted 
in the last patch of the series.
Don't use a link like that in the commit message especially when it's a
link to an email you wrote.  If it's someone else's email you can say,
something like "As <name> points out in <his/her> email <url>.  Blah
blah blah."  That way you give credit to the other person but all the
information is in the commit message.
I agree with you. I'll redo the commit message for in order to summarize in 
few lines why I'm doing clean-ups of functions that must be deleted in 18/18.
The same for 16/18. I think that a short explanation like the one that I gave 
you above should suffice (much shorter, obviously).

I hope that I've been clear now. Please let me know if you have more 
suggestions about this patch and the next (16/18).

Regards,

Fabio

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help