Re: setpriority(2): booleans are not scalars
David Riley wrote: > On Sep 25, 2020, at 3:13 PM, Todd C. Miller wrote: > > > > On Fri, 25 Sep 2020 13:58:01 -0500, Scott Cheloha wrote: > > > >> `found' serves as a boolean here. I'd prefer to simple and set it to > >> 1 instead of incrementing it when we find what we're looking for. > > > > Makes sense to me, the use of var++ instead of var=1 is old-school > > style ;-) > > There are still some architectures where an increment vs. an immediate load > is a shorter instruction, but I strongly suspect the performance difference > (where there even is one) is not worth the unclarity here. Which architecture is faster to load+increment+store, than store a constant? I'd like to know. But let's look deeper. How about we fix OpenBSD to support 2GB or 4GB of processes. What happens next? Alternatively on some architectures it is faster to increment a char than an int, how about we change var to 'char var = 0'? Still feeling good about the change? Premature optimization can be hazardous. The ++ code as written carries meanings and intents, which are incorrect.
Re: setpriority(2): booleans are not scalars
On Sep 25, 2020, at 3:13 PM, Todd C. Miller wrote: > > On Fri, 25 Sep 2020 13:58:01 -0500, Scott Cheloha wrote: > >> `found' serves as a boolean here. I'd prefer to simple and set it to >> 1 instead of incrementing it when we find what we're looking for. > > Makes sense to me, the use of var++ instead of var=1 is old-school > style ;-) There are still some architectures where an increment vs. an immediate load is a shorter instruction, but I strongly suspect the performance difference (where there even is one) is not worth the unclarity here. - Dave
Re: setpriority(2): booleans are not scalars
We can't have 2 billion processes to reach a wrap. But I agree, it seems quite wrong, and seems better to just observe match rather than count. Scott Cheloha wrote: > `found' serves as a boolean here. I'd prefer to simple and set it to > 1 instead of incrementing it when we find what we're looking for. > > ok? > > Index: kern_resource.c > === > RCS file: /cvs/src/sys/kern/kern_resource.c,v > retrieving revision 1.68 > diff -u -p -r1.68 kern_resource.c > --- kern_resource.c 15 Jul 2019 20:44:48 - 1.68 > +++ kern_resource.c 25 Sep 2020 18:54:34 - > @@ -157,7 +157,7 @@ sys_setpriority(struct proc *curp, void > if (pr == NULL) > break; > error = donice(curp, pr, SCARG(uap, prio)); > - found++; > + found = 1; > break; > > case PRIO_PGRP: { > @@ -169,7 +169,7 @@ sys_setpriority(struct proc *curp, void > break; > LIST_FOREACH(pr, &pg->pg_members, ps_pglist) { > error = donice(curp, pr, SCARG(uap, prio)); > - found++; > + found = 1; > } > break; > } > @@ -180,14 +180,14 @@ sys_setpriority(struct proc *curp, void > LIST_FOREACH(pr, &allprocess, ps_list) > if (pr->ps_ucred->cr_uid == SCARG(uap, who)) { > error = donice(curp, pr, SCARG(uap, prio)); > - found++; > + found = 1; > } > break; > > default: > return (EINVAL); > } > - if (found == 0) > + if (!found) > return (ESRCH); > return (error); > } >
Re: setpriority(2): booleans are not scalars
On Fri, 25 Sep 2020 13:58:01 -0500, Scott Cheloha wrote: > `found' serves as a boolean here. I'd prefer to simple and set it to > 1 instead of incrementing it when we find what we're looking for. Makes sense to me, the use of var++ instead of var=1 is old-school style ;-) - todd
setpriority(2): booleans are not scalars
`found' serves as a boolean here. I'd prefer to simple and set it to 1 instead of incrementing it when we find what we're looking for. ok? Index: kern_resource.c === RCS file: /cvs/src/sys/kern/kern_resource.c,v retrieving revision 1.68 diff -u -p -r1.68 kern_resource.c --- kern_resource.c 15 Jul 2019 20:44:48 - 1.68 +++ kern_resource.c 25 Sep 2020 18:54:34 - @@ -157,7 +157,7 @@ sys_setpriority(struct proc *curp, void if (pr == NULL) break; error = donice(curp, pr, SCARG(uap, prio)); - found++; + found = 1; break; case PRIO_PGRP: { @@ -169,7 +169,7 @@ sys_setpriority(struct proc *curp, void break; LIST_FOREACH(pr, &pg->pg_members, ps_pglist) { error = donice(curp, pr, SCARG(uap, prio)); - found++; + found = 1; } break; } @@ -180,14 +180,14 @@ sys_setpriority(struct proc *curp, void LIST_FOREACH(pr, &allprocess, ps_list) if (pr->ps_ucred->cr_uid == SCARG(uap, who)) { error = donice(curp, pr, SCARG(uap, prio)); - found++; + found = 1; } break; default: return (EINVAL); } - if (found == 0) + if (!found) return (ESRCH); return (error); }