| Message ID | YUlV+RPMHGGfk25d@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | const-correctness in grep.c | expand |
On Mon, Sep 20, 2021 at 11:48:09PM -0400, Jeff King wrote: > When showing lines via grep (or looking for funcnames), we call > show_line() on a multi-line buffer. It finds the end of line and marks > it with a NUL. However, we don't need to do so, as the resulting line is > only used along with its "eol" marker: > > - we pass both to next_match(), which takes care to look at only the > bytes we specified Thinking aloud, next_match() calls match_next_pattern() which takes eol as non-const and passes it to match_one_pattern(). And that calls strip_timestamp(), which would be non-const, were it not the previous patch. So I think this conversion is safe. > - we pass the line to output_color() without its matching eol marker. > However, we do use the "match" struct we got from next_match() to > tell it how many bytes to look at (which can never exceed the string > we passed it Yep, makes sense. The patch looks good and matches your description here. Thanks, Taylor
On Tue, Sep 21, 2021 at 12:22:45AM -0400, Taylor Blau wrote: > On Mon, Sep 20, 2021 at 11:48:09PM -0400, Jeff King wrote: > > When showing lines via grep (or looking for funcnames), we call > > show_line() on a multi-line buffer. It finds the end of line and marks > > it with a NUL. However, we don't need to do so, as the resulting line is > > only used along with its "eol" marker: > > > > - we pass both to next_match(), which takes care to look at only the > > bytes we specified > > Thinking aloud, next_match() calls match_next_pattern() which takes eol > as non-const and passes it to match_one_pattern(). And that calls > strip_timestamp(), which would be non-const, were it not the previous > patch. So I think this conversion is safe. To be a little nit-picky: this move would be OK even without the change to strip_timestamp(). The question is whether any of those sub-calls actually looks past the "eol" pointer we give it. I didn't dig all the way down through the complete call-stack in an exhaustive way. But after having looked at the related functions when changing their const-ness elsewhere in the series, they'd have to be ignoring the "eol" parameter for it to be a problem. The irony, of course, is that they did ignore this parameter at one point! Because they'd all eventually end in regexec(), which would happily read past our "eol". So really, all of this is predicated on those sub-functions behaving sensibly. I think they do. But if we found that they didn't, I'd much rather know that and fix them than live with this "this NUL may or may not be important" state forever. > > - we pass the line to output_color() without its matching eol marker. > > However, we do use the "match" struct we got from next_match() to > > tell it how many bytes to look at (which can never exceed the string > > we passed it > > Yep, makes sense. The patch looks good and matches your description > here. Thanks for looking it over (my nitpick aside). :) -Peff
On Tue, Sep 21, 2021 at 12:42:05AM -0400, Jeff King wrote: > On Tue, Sep 21, 2021 at 12:22:45AM -0400, Taylor Blau wrote: > > > On Mon, Sep 20, 2021 at 11:48:09PM -0400, Jeff King wrote: > > > When showing lines via grep (or looking for funcnames), we call > > > show_line() on a multi-line buffer. It finds the end of line and marks > > > it with a NUL. However, we don't need to do so, as the resulting line is > > > only used along with its "eol" marker: > > > > > > - we pass both to next_match(), which takes care to look at only the > > > bytes we specified > > > > Thinking aloud, next_match() calls match_next_pattern() which takes eol > > as non-const and passes it to match_one_pattern(). And that calls > > strip_timestamp(), which would be non-const, were it not the previous > > patch. So I think this conversion is safe. > > To be a little nit-picky: this move would be OK even without the change > to strip_timestamp(). The question is whether any of those sub-calls > actually looks past the "eol" pointer we give it. Right, I wasn't implying that this move was unsafe without the previous patch. Just that without the previous patch, we couldn't make show_line() take a const-pointer to eol. Thanks, Taylor
diff --git a/grep.c b/grep.c index 5b1f2da4d3..70af01d1c1 100644 --- a/grep.c +++ b/grep.c @@ -1239,7 +1239,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol, if (opt->color || opt->only_matching) { regmatch_t match; enum grep_context ctx = GREP_CONTEXT_BODY; - int ch = *eol; int eflags = 0; if (opt->color) { @@ -1254,7 +1253,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol, else if (sign == '=') line_color = opt->colors[GREP_COLOR_FUNCTION]; } - *eol = '\0'; while (next_match(opt, bol, eol, ctx, &match, eflags)) { if (match.rm_so == match.rm_eo) break; @@ -1272,7 +1270,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol, rest -= match.rm_eo; eflags = REG_NOTBOL; } - *eol = ch; } if (!opt->only_matching) { output_color(opt, bol, rest, line_color);
When showing lines via grep (or looking for funcnames), we call show_line() on a multi-line buffer. It finds the end of line and marks it with a NUL. However, we don't need to do so, as the resulting line is only used along with its "eol" marker: - we pass both to next_match(), which takes care to look at only the bytes we specified - we pass the line to output_color() without its matching eol marker. However, we do use the "match" struct we got from next_match() to tell it how many bytes to look at (which can never exceed the string we passed it). So we can stop setting and restoring this NUL marker. That makes the code simpler, and will allow us to take a const buffer in a future patch. Signed-off-by: Jeff King <peff@peff.net> --- grep.c | 3 --- 1 file changed, 3 deletions(-)