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
