Re: patch for review: xargs standards compliance

"J. Mallett" <[email protected]> Fri, 22 Mar 2002 15:12:15 +0000

Newsgroups	gmane.os.freebsd.devel.audit
Message-ID	<[email protected]>

On Fri, Mar 22, 2002 at 04:08:02PM +1100, Tim J. Robbins wrote:
> On Fri, Mar 22, 2002 at 03:52:31AM +0000, J. Mallett wrote:
> 
> > On Sat, Mar 16, 2002 at 10:14:35AM +0000, J. Mallett wrote:
> 
> > > http://people.freebsd.org/~jmallett/xargs.patch3
> > 
> > I take it nobody objects to this version of my patch then?
> 
> I don't like the wording here:
> 
> + .It Fl I Ar replstr
> + If this option is specified up to 5 occurences of
> + .Ar replstr
> + may be replaced in the arguments to
> + .Ar utility
> + with one line of standard input.
> + .Ar Utility
> + is executed for every line of input in this mode.
> + In this mode
> + .Fl x
> + is implied.
> 
> Wouldn't this be better?
> 
> .It Fl I Ar replstr
> Execute
> .Ar utility
> for each input line, replacing up to 5 occurrences of
> .Ar replstr
> in the arguments with the line read.
> Implies
> .Fl x .
> 
> (or similar; mdoc-ing hasn't been checked).

Good call on that, I believe, I've used that as a jumping off point to 
redo -I's description, though I think it needs to be clear that it's 
arguments to .Ar utility that will have replacement done..

> 
> And while not really a bug, there probably should be const qualifiers
> on `match' and `replstr' in strnsubst():
> size_t strnsubst(char **str, char *match, char *replstr, size_t n)

Fair enough, my local copy of strnsubst has been cleaned with such, so I 
have no problems with that.

> 
> + 		if (this == NULL)
> + 			goto done;
> 
> This goto could be changed to a break, simplifying it a bit.

Sounds reasonable.

> 
> Tim

Well, given all the input thus far, I'd like to commit
	http://people.freebsd.org/~jmallett/xargs.patch4

Some time this evening, as I've handled every comment I've gotten in the 
past week of review (I believe).

Thank you,
	/j.

signature.asc (application/pgp-signature, 290 B)