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

From: Jeff King <peff@peff.net>
Date: Jun 19, 2018, 16:28 UTC
Message-ID: <20180619162825.GA22034@sigill.intra.peff.net>
In-Reply-To: <df2d08efc1fb717a564157fb760ff71becb658dd.1529365072.git.me@ttaylorr.com>

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"?

But ultimately, the column number comes from this code:

Show 21 quoted lines

> @@ -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.

-Peff