Re: [patch] /bin/cp: reduce scope variable
>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
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
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
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
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
[patch] /bin/cp: reduce scope variable
Hi, Reduce scope of a few variables. While here, remove an extraneous space. Regards, diff --git a/bin/cp/utils.c b/bin/cp/utils.c index a2f480c3393..3e2f94e4fa4 100644 --- a/bin/cp/utils.c +++ b/bin/cp/utils.c @@ -55,10 +55,7 @@ copy_file(FTSENT *entp, int exists) static char *buf; static char *zeroes; struct stat to_stat, *fs; - int from_fd, rcount, rval, to_fd, wcount; -#ifdef VM_AND_BUFFER_CACHE_SYNCHRONIZED - char *p; -#endif + int from_fd, rval, to_fd; if (!buf) { buf = malloc(MAXBSIZE); @@ -96,7 +93,7 @@ copy_file(FTSENT *entp, int exists) if (!copy_overwrite()) { (void)close(from_fd); return 2; - } + } to_fd = open(to.p_path, O_WRONLY | O_TRUNC, 0); } else to_fd = open(to.p_path, O_WRONLY | O_TRUNC | O_CREAT, @@ -118,6 +115,7 @@ copy_file(FTSENT *entp, int exists) #ifdef VM_AND_BUFFER_CACHE_SYNCHRONIZED /* XXX broken for 0-size mmap */ if (fs->st_size <= 8 * 1048576) { + char *p; if ((p = mmap(NULL, (size_t)fs->st_size, PROT_READ, MAP_FILE|MAP_SHARED, from_fd, (off_t)0)) == MAP_FAILED) { warn("mmap: %s", entp->fts_path); @@ -138,6 +136,7 @@ copy_file(FTSENT *entp, int exists) #endif { int skipholes = 0; + int rcount, wcount; struct stat tosb; if (!fstat(to_fd, &tosb) && S_ISREG(tosb.st_mode)) skipholes = 1; @@ -255,9 +254,8 @@ copy_special(struct stat *from_stat, int exists) int copy_overwrite(void) { - int ch, checkch; - if (iflag) { + int ch, checkch; (void)fprintf(stderr, "overwrite %s? ", to.p_path); checkch = ch = getchar(); while (ch != '\n' && ch != EOF) -- Jerome Pinot