On Mon, Sep 20, 2021 at 09:24:08PM -0400, Jeff King wrote:
> > + buf = (char *)line;
> > + eol = buf + linelen;
>
> OK, so we got rid of the copy of "line", which is nice. But we are
> casting away const-ness, which is a potential red flag (is somebody
> going to modify this string, even though we promised our caller we would
> not?). We'd probably want a comment to explain why we are doing so, and
> why it is OK (e.g., if somebody in the call stack modifies it
> temporarily).
>
> More on this in a moment.
The root of the issue is that grep_next_match() takes a non-const
buffer, and so on. And indeed, it _does_ eventually get modified,
although only temporarily. I think we can clean that up, though.
Here are two patches I prepared on top of your series to show what's
possible, though I think we should do one of:
- put them at the front of your series (with the appropriate
adjustments) as preparatory cleanup
- keep them separate. You can put a comment above the cast to mention
what's going on and why it's OK for now, and then later when they're
both merged, we can remove that cast.
The second option creates a little extra work for the maintainer (they
both touch match_one_patter(), so there will be some textual conflicts).
But it does mean we avoid a dependencies; the cleanups don't derail your
series, nor does your series hold up the cleanups. So I could go either
way.
[1/2]: grep: stop modifying buffer in strip_timestamp
[2/2]: grep: mark "haystack" buffers as const
grep.c | 30 ++++++++++++------------------
grep.h | 3 ++-
pretty.c | 6 +++---
3 files changed, 17 insertions(+), 22 deletions(-)
-Peff
