Re: [PATCH blktests v1 01/11] nvme/{003,004,005,013,046,049}: Group all variables declarations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Jul 27, 2023 / 08:18, Bart Van Assche wrote:
> On 7/27/23 00:11, Daniel Wagner wrote:
> > On Wed, Jul 26, 2023 at 07:54:24AM -0700, Bart Van Assche wrote:
> > > On 7/26/23 05:46, Daniel Wagner wrote:
> > > > Group all variable declarations together at the beginning of the
> > > > function.
> > > 
> > > An explanation of why this change has been proposed is missing from the
> > > patch description.
> > 
> > Sure, I'll add one. The coding style to declare all local variables at the
> > beginning of the function.
> 
> Isn't declaring local variables just before their first use a better style?

IMO both styles have pros and cons. Declarations at "beginning of functions"
helps to understand what the function uses as its local data (pros), but the
declaration and the usage are separated and makes it difficult to understand
(cons). Declarations at "just before first use" have the opposite pros and cons.
This style is easier to read especially when a function is rather long.

In the past, I preferred declarations at the beginning functions and requested
it in my review comments [1], but I learned that this guide is not so widely
applied: xfstests scripts, or even blktests 'check' scripts have declarations in
the middle of the functions. So I think both styles are okay at this moment.

  [1] https://github.com/osandov/blktests/pull/99

More importantly, this discussion maybe going towards "too strict" guidelines,
which will discourage contributions. Similar topic is [[ ]] vs [ ]. Once I was
requesting strictly to use [[ ]], but it did not seem productive. Now I no
longer request to replace [ ] with [[ ]]. In same manner, I suggest not to be
strict on the local variable declaration position either.

As for this patch, it is not required to follow guidelines. Does it make
Daniel's refactoring work easier? If so, I guess it will be valuable.




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux