Re: [patch] basename(1) tweaks
>> - activate stack protector > > Hm? Changing the exit to a return does this? yes, stack protector only works if the function returns.
Re: [patch] basename(1) tweaks
frit...@alokat.org wrote:
> Hi tech@,
>
> here are some basename(1) tweaks:
> - remove (void) cast for puts(3) and fprintf(3)
We typically leave these. I agree that they're usually pointless visual
distractions in 2015 because there are a set of functions (mostly
blocking IO functions to stdout and stderr) that are assumed not to fail
unless misused, pretty much as as an API guarantee.
Removing them might cause more churn than it's worth though. I'd
probably save these for code that's being actively developed and needs
the readability improvement.
> - activate stack protector
Hm? Changing the exit to a return does this?
> - put includes in correct order
ok mmcc@
> Index: basename.c
> ===
> RCS file: /cvs/src/usr.bin/basename/basename.c,v
> retrieving revision 1.11
> diff -u -r1.11 basename.c
> --- basename.c9 Oct 2015 01:37:06 - 1.11
> +++ basename.c24 Dec 2015 16:33:35 -
> @@ -32,10 +32,10 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> -#include
> #include
>
> void usage(void);
> @@ -64,7 +64,7 @@
> usage();
>
> if (**argv == '\0') {
> - (void)puts("");
> + puts("");
> exit(0);
> }
> p = basename(*argv);
> @@ -88,8 +88,8 @@
> p[off] = '\0';
> }
> }
> - (void)puts(p);
> - exit(0);
> + puts(p);
> + return 0;
> }
>
> extern char *__progname;
> @@ -97,6 +97,6 @@
> usage(void)
> {
>
> - (void)fprintf(stderr, "usage: %s string [suffix]\n", __progname);
> + fprintf(stderr, "usage: %s string [suffix]\n", __progname);
> exit(1);
> }
>
[patch] basename(1) tweaks
Hi tech@,
here are some basename(1) tweaks:
- remove (void) cast for puts(3) and fprintf(3)
- activate stack protector
- put includes in correct order
--F.
Index: basename.c
===
RCS file: /cvs/src/usr.bin/basename/basename.c,v
retrieving revision 1.11
diff -u -r1.11 basename.c
--- basename.c 9 Oct 2015 01:37:06 - 1.11
+++ basename.c 24 Dec 2015 16:33:35 -
@@ -32,10 +32,10 @@
#include
#include
+#include
#include
#include
#include
-#include
#include
void usage(void);
@@ -64,7 +64,7 @@
usage();
if (**argv == '\0') {
- (void)puts("");
+ puts("");
exit(0);
}
p = basename(*argv);
@@ -88,8 +88,8 @@
p[off] = '\0';
}
}
- (void)puts(p);
- exit(0);
+ puts(p);
+ return 0;
}
extern char *__progname;
@@ -97,6 +97,6 @@
usage(void)
{
- (void)fprintf(stderr, "usage: %s string [suffix]\n", __progname);
+ fprintf(stderr, "usage: %s string [suffix]\n", __progname);
exit(1);
}
Re: [patch] basename(1) tweaks
Theo de Raadt wrote:
> I don't know if I am alone in this -- I am getting a bit tired of
> changes which are can be summarized as: "fighting someone else's
> style".
>
> I do not really see the value in changes of this sort.
Agreed. I ordered the includes, but the void casts should just stay.
I'd argue that some style changes make static analysis a little easier
or greatly improve readability. It's a fine line.
> >frit...@alokat.org wrote:
> >> Hi tech@,
> >>
> >> here are some basename(1) tweaks:
> >> - remove (void) cast for puts(3) and fprintf(3)
> >
> >We typically leave these. I agree that they're usually pointless visual
> >distractions in 2015 because there are a set of functions (mostly
> >blocking IO functions to stdout and stderr) that are assumed not to fail
> >unless misused, pretty much as as an API guarantee.
> >
> >Removing them might cause more churn than it's worth though. I'd
> >probably save these for code that's being actively developed and needs
> >the readability improvement.
> >
> >> - activate stack protector
> >
> >Hm? Changing the exit to a return does this?
> >
> >> - put includes in correct order
> >
> >ok mmcc@
> >
> >> Index: basename.c
> >> ===
> >> RCS file: /cvs/src/usr.bin/basename/basename.c,v
> >> retrieving revision 1.11
> >> diff -u -r1.11 basename.c
> >> --- basename.c 9 Oct 2015 01:37:06 - 1.11
> >> +++ basename.c 24 Dec 2015 16:33:35 -
> >> @@ -32,10 +32,10 @@
> >>
> >> #include
> >> #include
> >> +#include
> >> #include
> >> #include
> >> #include
> >> -#include
> >> #include
> >>
> >> void usage(void);
> >> @@ -64,7 +64,7 @@
> >>usage();
> >>
> >>if (**argv == '\0') {
> >> - (void)puts("");
> >> + puts("");
> >>exit(0);
> >>}
> >>p = basename(*argv);
> >> @@ -88,8 +88,8 @@
> >>p[off] = '\0';
> >>}
> >>}
> >> - (void)puts(p);
> >> - exit(0);
> >> + puts(p);
> >> + return 0;
> >> }
> >>
> >> extern char *__progname;
> >> @@ -97,6 +97,6 @@
> >> usage(void)
> >> {
> >>
> >> - (void)fprintf(stderr, "usage: %s string [suffix]\n", __progname);
> >> + fprintf(stderr, "usage: %s string [suffix]\n", __progname);
> >>exit(1);
> >> }
> >>
> >
> >
Re: [patch] basename(1) tweaks
>Theo de Raadt wrote: >> I don't know if I am alone in this -- I am getting a bit tired of >> changes which are can be summarized as: "fighting someone else's >> style". >> >> I do not really see the value in changes of this sort. > >Agreed. I ordered the includes, but the void casts should just stay. > >I'd argue that some style changes make static analysis a little easier >or greatly improve readability. It's a fine line. Well that's tough. The source files are not machine generated. People wrote them. Some of them had inconsistent style. Others followed stronger patterns, but patterns changed over time. There is little point in churning this code year by new into a new pattern which brings no benefits, and it is just pointless churn. Why not spit in the face of the original author at the same time? If static analysis tools cannot deal with this, too bad. In this program, really?? Get serious.
Re: [patch] basename(1) tweaks
I don't know if I am alone in this -- I am getting a bit tired of
changes which are can be summarized as: "fighting someone else's style".
I do not really see the value in changes of this sort.
>frit...@alokat.org wrote:
>> Hi tech@,
>>
>> here are some basename(1) tweaks:
>> - remove (void) cast for puts(3) and fprintf(3)
>
>We typically leave these. I agree that they're usually pointless visual
>distractions in 2015 because there are a set of functions (mostly
>blocking IO functions to stdout and stderr) that are assumed not to fail
>unless misused, pretty much as as an API guarantee.
>
>Removing them might cause more churn than it's worth though. I'd
>probably save these for code that's being actively developed and needs
>the readability improvement.
>
>> - activate stack protector
>
>Hm? Changing the exit to a return does this?
>
>> - put includes in correct order
>
>ok mmcc@
>
>> Index: basename.c
>> ===
>> RCS file: /cvs/src/usr.bin/basename/basename.c,v
>> retrieving revision 1.11
>> diff -u -r1.11 basename.c
>> --- basename.c 9 Oct 2015 01:37:06 - 1.11
>> +++ basename.c 24 Dec 2015 16:33:35 -
>> @@ -32,10 +32,10 @@
>>
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> -#include
>> #include
>>
>> void usage(void);
>> @@ -64,7 +64,7 @@
>> usage();
>>
>> if (**argv == '\0') {
>> -(void)puts("");
>> +puts("");
>> exit(0);
>> }
>> p = basename(*argv);
>> @@ -88,8 +88,8 @@
>> p[off] = '\0';
>> }
>> }
>> -(void)puts(p);
>> -exit(0);
>> +puts(p);
>> +return 0;
>> }
>>
>> extern char *__progname;
>> @@ -97,6 +97,6 @@
>> usage(void)
>> {
>>
>> -(void)fprintf(stderr, "usage: %s string [suffix]\n", __progname);
>> +fprintf(stderr, "usage: %s string [suffix]\n", __progname);
>> exit(1);
>> }
>>
>
>
