Re: [PATCH 4/7] grep.c: display column number of first match

[Taylor Blau <me@ttaylorr.com>]

On Tue, Jun 19, 2018 at 12:28:26PM -0400, Jeff King wrote:
> On Mon, Jun 18, 2018 at 06:43:14PM -0500, Taylor Blau wrote:
>
> >  static void show_line(struct grep_opt *opt, char *bol, char *eol,
> > -		      const char *name, unsigned lno, char sign)
> > +		      const char *name, unsigned lno, unsigned cno, char sign)
>
> Here "cno" is unsigned. But later...
>
> > +	if (opt->columnnum && cno) {
> > +		char buf[32];
> > +		xsnprintf(buf, sizeof(buf), "%d", cno);
>
> ...we print it with "%d". Should this be "%u"?

Thanks, that's certainly a mistake. I think (per the hunk of this
response below) that it should be "%zu" in the case that we change this
patch to take an ssize_t.

> But ultimately, the column number comes from this code:
>
> > @@ -1785,6 +1796,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
> >  	while (left) {
> >  		char *eol, ch;
> >  		int hit;
> > +		ssize_t cno;
> >  		ssize_t col = -1, icol = -1;
> >
> >  		/*
> > @@ -1850,7 +1862,15 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
> >  				show_pre_context(opt, gs, bol, eol, lno);
> >  			else if (opt->funcname)
> >  				show_funcname_line(opt, gs, bol, lno);
> > -			show_line(opt, bol, eol, gs->name, lno, ':');
> > +			cno = opt->invert ? icol : col;
> > +			if (cno < 0) {
> > +				/*
> > +				 * A negative cno means that there was no match.
> > +				 * Clamp to the beginning of the line.
> > +				 */
> > +				cno = 0;
> > +			}
>
> ...which is a ssize_t. Should we just be using ssize_t consistently?
>
> We do at least clamp the negative values here, but on 64-bit systems
> ssize_t is much larger than "unsigned".  I admit that it's probably
> ridiculous for any single line to overflow 32 bits, but it seems like we
> should consistently use size_t/ssize_t for buffer offsets, and then we
> don't have to think about it.

I agree that it's unlikely that a single line will overflow 32 bits, and
certainly at that point we might have other problems to worry about :-).

This was an unsigned in my original patch, and I left it this way in the
revised series for consistency with the other arguments to show_line().
But, I agree with your reasoning and think that this should be an
ssize_t, instead.


Thanks,
Taylor