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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 23 Feb 2017 14:38:13 +0100 (CET)
From:   Julia Lawall <julia.lawall@...6.fr>
To:     Arend Van Spriel <arend.vanspriel@...adcom.com>
cc:     Julia Lawall <julia.lawall@...6.fr>,
        Tahia Khan <tahia.khan@...il.com>,
        outreachy-kernel@...glegroups.com, aditya.shankar@...rochip.com,
        ganesh.krishna@...rochip.com, gregkh@...uxfoundation.org,
        linux-wireless@...r.kernel.org, devel@...verdev.osusl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [Outreachy kernel] Re: [PATCH v2] staging: wilc1000: renames
 struct tstrRSSI and its members u8Index, u8Full



On Thu, 23 Feb 2017, 'Arend Van Spriel' via outreachy-kernel wrote:

> 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.

OK.  I guess I mainly saw the change of type as being different.

julia


>
> Regards,
> Arend
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@...glegroups.com.
> To post to this group, send email to outreachy-kernel@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1ffdd756-aaa4-5e1e-b72f-3e5d1f2daeb2%40broadcom.com.
> For more options, visit https://groups.google.com/d/optout.
>