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