Re: [patch] /bin/cp: reduce scope variable

2020-02-02 Thread Theo de Raadt
>On Sat, Feb 01, 2020 at 04:57:26PM +0100, Ingo Schwarze wrote:
>> Hi Jeremie,
>> 
>> Jeremie Courreges-Anglas wrote on Sat, Feb 01, 2020 at 01:37:32PM +0100:
>> > On Fri, Jan 31 2020, Ingo Schwarze  wrote:
>> >> ngc...@gmail.com wrote on Fri, Jan 31, 2020 at 10:14:52PM +0900:
>> 
>> >>> Reduce scope of a few variables.
>> 
>> >> No, this contradicts OpenBSD coding style.
>> >> We want local variables declared up front in functions
>> >> such that you can see at one glance which variables exist.

Disagree very strongly against that >> >> statement, the counterpoint
would be hey let's make everything global it is the master scope!!!

>> > Huh, this is your personal preference and I strongly disagree with
>> > making it an "official" stance (again).  Reducing the scope of variables
>> > *quite often* helps reasoning about them.
>> > 
>> > style(9) said something like that, and has been changed three years
>> > ago (for good IMO).
>> > 
>> >   revision 1.68
>> >   date: 2016/10/18 18:13:56;  author: millert;  state: Exp;  lines: +2 -4; 
>> >  commitid: aPPHgmmA4hseZUFx;
>> >   Don't tell the programmer not to put variable declarations inside
>> >   blocks.  OK guenther@ kettenis@
>> 
>> Oops.  Sorry for forgetting that the recommendation was dropped and
>> for making you dig up the commit and thanks for reminding me.
>
>Since it came out in the open, I also like declaring variables as late
>as possible. Specifically, because the old-style (declare variables early,
>then give them a value when possible) tends to be unsafe. You forget about
>initializing variables, or you initialize them to something that doesn't
>make sense.
>
>I also don't think it's more readable to have every variable at the start
>of the function.   The less space my eyes have to travel the better.
>Especially with functions that run 50 lines long.
>
>Also, reducing the scope of variables tends to be one of the first steps
>in untangling old atrocious code: reduce scope, figure out you got two
>separate blocks in your function with totally separate variables, and then
>you write a new function.
>
>Compilers aren't perfect yet, but we still deal with code that was written
>20-30 years ago, back when they were even worse, and a lot of dirty tricks
>that were used back then are no longer needed.  Reusing variables, having
>the compiler match your scopes in asm exactly, you name it. These days,
>scope is only a semantic promise that this variable only matters in THAT
>specific block of code. The stronger the promise, the more readable 
>the code.

100% agreed, narrow scoping is obviously a safer practice.

but is cat the place to start in what appears to be a training wheels process?

and i agree with jca, diffs with additional churn which must be
verified or removed are just not as welcome.  the odds of the
confusing churn in the diff introducing a bug are higher until it is
verified or rewritten, gah who has time for that shit.



Re: [patch] /bin/cp: reduce scope variable

2020-02-02 Thread Marc Espie
On Sat, Feb 01, 2020 at 04:57:26PM +0100, Ingo Schwarze wrote:
> Hi Jeremie,
> 
> Jeremie Courreges-Anglas wrote on Sat, Feb 01, 2020 at 01:37:32PM +0100:
> > On Fri, Jan 31 2020, Ingo Schwarze  wrote:
> >> ngc...@gmail.com wrote on Fri, Jan 31, 2020 at 10:14:52PM +0900:
> 
> >>> Reduce scope of a few variables.
> 
> >> No, this contradicts OpenBSD coding style.
> >> We want local variables declared up front in functions
> >> such that you can see at one glance which variables exist.
> 
> > Huh, this is your personal preference and I strongly disagree with
> > making it an "official" stance (again).  Reducing the scope of variables
> > *quite often* helps reasoning about them.
> > 
> > style(9) said something like that, and has been changed three years
> > ago (for good IMO).
> > 
> >   revision 1.68
> >   date: 2016/10/18 18:13:56;  author: millert;  state: Exp;  lines: +2 -4;  
> > commitid: aPPHgmmA4hseZUFx;
> >   Don't tell the programmer not to put variable declarations inside
> >   blocks.  OK guenther@ kettenis@
> 
> Oops.  Sorry for forgetting that the recommendation was dropped and
> for making you dig up the commit and thanks for reminding me.

Since it came out in the open, I also like declaring variables as late
as possible. Specifically, because the old-style (declare variables early,
then give them a value when possible) tends to be unsafe. You forget about
initializing variables, or you initialize them to something that doesn't
make sense.

I also don't think it's more readable to have every variable at the start
of the function.   The less space my eyes have to travel the better.
Especially with functions that run 50 lines long.

Also, reducing the scope of variables tends to be one of the first steps
in untangling old atrocious code: reduce scope, figure out you got two
separate blocks in your function with totally separate variables, and then
you write a new function.

Compilers aren't perfect yet, but we still deal with code that was written
20-30 years ago, back when they were even worse, and a lot of dirty tricks
that were used back then are no longer needed.  Reusing variables, having
the compiler match your scopes in asm exactly, you name it. These days,
scope is only a semantic promise that this variable only matters in THAT
specific block of code. The stronger the promise, the more readable 
the code.



Re: [patch] /bin/cp: reduce scope variable

2020-02-01 Thread Ingo Schwarze
Hi Jeremie,

Jeremie Courreges-Anglas wrote on Sat, Feb 01, 2020 at 01:37:32PM +0100:
> On Fri, Jan 31 2020, Ingo Schwarze  wrote:
>> ngc...@gmail.com wrote on Fri, Jan 31, 2020 at 10:14:52PM +0900:

>>> Reduce scope of a few variables.

>> No, this contradicts OpenBSD coding style.
>> We want local variables declared up front in functions
>> such that you can see at one glance which variables exist.

> Huh, this is your personal preference and I strongly disagree with
> making it an "official" stance (again).  Reducing the scope of variables
> *quite often* helps reasoning about them.
> 
> style(9) said something like that, and has been changed three years
> ago (for good IMO).
> 
>   revision 1.68
>   date: 2016/10/18 18:13:56;  author: millert;  state: Exp;  lines: +2 -4;  
> commitid: aPPHgmmA4hseZUFx;
>   Don't tell the programmer not to put variable declarations inside
>   blocks.  OK guenther@ kettenis@

Oops.  Sorry for forgetting that the recommendation was dropped and
for making you dig up the commit and thanks for reminding me.

Yours
  Ingo



Re: [patch] /bin/cp: reduce scope variable

2020-02-01 Thread Jeremie Courreges-Anglas
On Fri, Jan 31 2020, Ingo Schwarze  wrote:
> Hi,
>
> ngc...@gmail.com wrote on Fri, Jan 31, 2020 at 10:14:52PM +0900:
>
>> Reduce scope of a few variables.
>
> No, this contradicts OpenBSD coding style.
> We want local variables declared up front in functions
> such that you can see at one glance which variables exist.

Huh, this is your personal preference and I strongly disagree with
making it an "official" stance (again).  Reducing the scope of variables
*quite often* helps reasoning about them.

style(9) said something like that, and has been changed three years
ago (for good IMO).

  revision 1.68
  date: 2016/10/18 18:13:56;  author: millert;  state: Exp;  lines: +2 -4;  
commitid: aPPHgmmA4hseZUFx;
  Don't tell the programmer not to put variable declarations inside
  blocks.  OK guenther@ kettenis@

This being said, the patch doesn't apply and the proposed change doesn't
improve the current code much, so this feels like needless churn.
"ngc891", please find another itch to scratch. :)

>> While here, remove an extraneous space.
>
> While avoiding trailing whitespace is good, it's not worth
> a commit (nor sending patches around).

Seconded.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE



Re: [patch] /bin/cp: reduce scope variable

2020-01-31 Thread Ingo Schwarze
Hi,

ngc...@gmail.com wrote on Fri, Jan 31, 2020 at 10:14:52PM +0900:

> Reduce scope of a few variables.

No, this contradicts OpenBSD coding style.
We want local variables declared up front in functions
such that you can see at one glance which variables exist.

> While here, remove an extraneous space.

While avoiding trailing whitespace is good, it's not worth
a commit (nor sending patches around).

Yours,
  Ingo