Re: [PATCH 4/7] grep.c: display column number of first match
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jun 19, 2018, 16:34 UTC
- Message-ID
- <20180619163414.GA27246@syl.attlocal.net>
- In-Reply-To
- <20180619162825.GA22034@sigill.intra.peff.net>
On Tue, Jun 19, 2018 at 12:28:26PM -0400, Jeff King wrote:
Show 13 quoted lines
> 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.
Show 31 quoted lines
> 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