On 6/9/2015 1:28 AM, Bruce Evans wrote: > On Mon, 8 Jun 2015, Bryan Drewery wrote: > >> Log: >> Cleanup some style(9) issues. >> >> - Whitespace. >> - Comments. >> - Wrap long lines. > > cp's style had a remarlable amount of bitrot. > > This change unimproves it in some places.
I have been traveling and packing. I'm replying now but won't have time
to address the issues until next week. I was trying to avoid doing any
of this but touched code which was horrendously misstyled and chained
into reindenting the whole file and doing it wrong :). At this point I
don't want to tweak this much more.
>> --- head/bin/cp/cp.c Mon Jun 8 19:13:04 2015 (r284162)
>> +++ head/bin/cp/cp.c Mon Jun 8 19:24:18 2015 (r284163)
>> @@ -75,8 +75,8 @@ __FBSDID("$FreeBSD$");
>> #include "extern.h"
>>
>> #define STRIP_TRAILING_SLASH(p) { \
>> - while ((p).p_end > (p).p_path + 1 && (p).p_end[-1] == '/') \
>> - *--(p).p_end = 0; \
>> + while ((p).p_end > (p).p_path + 1 && (p).p_end[-1] == '/') \
>> + *--(p).p_end = 0; \
>
Woops.
>
>> @@ -245,10 +245,10 @@ main(int argc, char *argv[])
>> type = FILE_TO_FILE;
>>
>> if (have_trailing_slash && type == FILE_TO_FILE) {
>> - if (r == -1)
>> + if (r == -1) {
>
> This adds excessive braces.
>
>> errx(1, "directory %s does not exist",
>> - to.p_path);
>> - else
>> + to.p_path);
>> + } else
It is a multi-line statement due to the hard 80-width wrap. I feel it is
fine in this case.
>> errx(1, "%s is not a directory", to.p_path);
>> }
>> } else
>> ...
>> @@ -379,7 +379,8 @@ copy(char *argv[], enum op type, int fts
>> mode = curr->fts_statp->st_mode;
>> if ((mode & (S_ISUID | S_ISGID | S_ISTXT)) ||
>> ((mode | S_IRWXU) & mask) != (mode & mask))
>> - if (chmod(to.p_path, mode & mask) != 0){
>> + if (chmod(to.p_path, mode & mask) !=
>> + 0) {
>> warn("chmod: %s", to.p_path);
>> rval = 1;
>> }
>
> This changes from a minor misformatting to avoid a long line to even uglier
> formatting with a split line.
I agree 100%. I did it because of our hard 80-width cut-off. What would
the proper style be? My inclination would be to wrap at the first comma
but then it is even more odd. I find our 80-width cut-off to be strange
when editors/tmux/window manager/etc can resize and wrap long lines already.
Actually I don't see a width restriction in style(9) at all but surely
we have this rule documented somewhere. My guess is that it is inherited
by KNF.
> It is necessary to make such changes if you
> use indent(1) to generate and check the changes -- otherwise, indent keeps
Do you have an indent configuration I can use?
> reporting the misformatting -- but since cp rarely went near indent it
> may be better to keep its minor misformattings.
>
>> Modified: head/bin/cp/utils.c
>> ==============================================================================
>>
>> --- head/bin/cp/utils.c Mon Jun 8 19:13:04 2015 (r284162)
>> +++ head/bin/cp/utils.c Mon Jun 8 19:24:18 2015 (r284163)
>> ...
>> -/* Small (default) buffer size in bytes. It's inefficient for this to be
>> - * smaller than MAXPHYS */
>> +/*
>> + * Small (default) buffer size in bytes. It's inefficient for this to be
>> + * smaller than MAXPHYS.
>> + */
>
> Still has unusual sentence break of 1 space. cp uses normal sentence
> breaks
I did a minimal effort on comments and didn't clean up grammar or
breaks. I have not adopted 2 space breaks into my style(9) conformation yet.
>> @@ -345,7 +352,7 @@ setfile(struct stat *fs, int fd)
>> fdval = fd != -1;
>> islink = !fdval && S_ISLNK(fs->st_mode);
>> fs->st_mode &= S_ISUID | S_ISGID | S_ISVTX |
>> - S_IRWXU | S_IRWXG | S_IRWXO;
>> + S_IRWXU | S_IRWXG | S_IRWXO;
>
> Here the formatting was reasonable, but it was in gnu style and was hard to
> maintain since it is not supported by indent(1). It is still hard to
> maintain,
> since it has fancy splitting earlier than necessary to put the S_IS* and
> S_IR* parts of the expressions on separate lines. indent(1) cannot
> reproduce
> this splitting. Also, with the normal indentation of the condinuation
> line,
> the fancy splitting is not so readable.
I'm do not see how this was proper before or how it is worse now. The
indentation is tabs and then 4 spaces. I don't see exceptions to this in
style(9) or in other code.
>
>> @@ -543,8 +550,10 @@ usage(void)
>> {
>>
>> (void)fprintf(stderr, "%s\n%s\n",
>> -"usage: cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpsvx] source_file
>> target_file",
>> -" cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpsvx] source_file
>> ... "
>> -"target_directory");
>> + "usage: cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpsvx] "
>> + "source_file target_file",
>> + " cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpsvx] "
>> + "source_file ... "
>> + "target_directory");
>> exit(EX_USAGE);
>> }
>
> This breaks the careful outdentation and obfuscates the strings. The
Again, this broke the 80-width limit. I preferred the old way but was
going on down the 80-width line on my screen fixing violations.
I suggest we update our styles to not require this awful wrapping. It
makes `grep -r` very difficult when strings are split up. Perhaps I am
mistaken on the rule but we have a lot of code that needlessly wraps early.
Regards,
Bryan Drewery
signature.asc
Description: OpenPGP digital signature
