Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1586754

Re: [Outreachy kernel] Re: [PATCH v2] staging: wilc1000: renames struct tstrRSSI and its members u8Index, u8Full

Show all headers | View raw

On 23-2-2017 8:08, Julia Lawall wrote:
>> Thanks for the feedback Arend, I really appreciate it. I've decided to go with
>> these changes in my follow-up patch request:
>>
>> - rename tstrRSSI to 'rssi_history_buffer' as Aren suggested since it makes the
>> purpose of the struct clear
>> - remove Hungarian notation from all tstrRSSI members' names
>> - change type of u8Full to bool since it's only ever 1 or 0
>> - change name of as8RSSI to 'samples' since this buffer is only ever used to
>> compute an average, and the "rssi" prefix is implied by the struct's name
>> - rename str_rssi to rssi_history in the network_info struct for clarity
>>
>> Since my reasoning for these changes deviates from just "renaming to
>> avoid camel casing" (as in the original checkpatch.pl warning), would it still
>> make sense to submit all this in a single patch? I know my commit message
>> needs to change but I wonder if this is too much detail.
> 
> I would strongly suggest not to do it all in a single patch.  Even if these
> changes are not very complicated conceptually, there is always a chance of
> doing things wrong.  Taking the problems one by one will improve the chance
> that the result is correct.  Also, the results will be easier for you and
> others to review if each patch only does one thing.  And easier to revert
> if needed later if something goes wrong.

It is all related to cleaning up stuff in a single struct which I
consider "one thing" here. To me it looks a bit silly if you rename one
struct member when it is obvious that the other two need to be renamed
as well. The only somewhat sensible split I see here is: 1) rename the
struct itself, 2) rename the struct members, and 3) rename str_rssi
member in struct network_info.

Regards,
Arend

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2] staging: wilc1000: renames struct tstrRSSI and its  members u8Index, u8Full Tahia Khan <tahia.khan@gmail.com> - 2017-02-22 18:20 +0100
  Re: [PATCH v2] staging: wilc1000: renames struct tstrRSSI and its  members u8Index, u8Full Arend Van Spriel <arend.vanspriel@broadcom.com> - 2017-02-22 21:00 +0100
    Re: [PATCH v2] staging: wilc1000: renames struct tstrRSSI and its  members u8Index, u8Full Tahia Khan <tahia.khan@gmail.com> - 2017-02-23 05:10 +0100
      Re: [Outreachy kernel] Re: [PATCH v2] staging: wilc1000: renames  struct tstrRSSI and its members u8Index, u8Full Julia Lawall <julia.lawall@lip6.fr> - 2017-02-23 08:10 +0100
        Re: [Outreachy kernel] Re: [PATCH v2] staging: wilc1000: renames  struct tstrRSSI and its members u8Index, u8Full Arend Van Spriel <arend.vanspriel@broadcom.com> - 2017-02-23 10:00 +0100
          Re: [Outreachy kernel] Re: [PATCH v2] staging: wilc1000: renames  struct tstrRSSI and its members u8Index, u8Full Julia Lawall <julia.lawall@lip6.fr> - 2017-02-23 14:50 +0100
    Re: [PATCH v2] staging: wilc1000: renames struct tstrRSSI and its  members u8Index, u8Full Joe Perches <joe@perches.com> - 2017-02-23 07:30 +0100

csiph-web