On Sun, Jun 14, 2020 at 02:35:06PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@xxxxxxxxx> writes:
>
> > +#define control_packet_write(fd, s, errstr) \
> > + do { \
> > + (void)s"is a string constant"; \
> > + packet_trace_str((s), 1); \
> > + if (write_str_in_full((fd), (s)) < 0) \
> > + die_errno((errstr)); \
> > + } while (0)
> > +
>
> Oh, that's much better. If you go this route, drop your use of
> write_str_in_full(), but count the length of s with strlen() here
> to give the chance to the compilers to count the constant strings
> at compile time.
>
> > /*
> > * If we buffered things up above (we don't, but we should),
> > * we'd flush it here
> > */
> > void packet_flush(int fd)
> > {
> > - packet_trace("0000", 4, 1);
> > - if (write_in_full(fd, "0000", 4) < 0)
> > - die_errno(_("unable to write flush packet"));
> > + control_packet_write(fd, "0000", _("unable to write flush packet"));
>
> > +#define control_packet_buf_write(buf, s) \
> > + do { \
> > + (void)s"is a string constant"; \
> > + packet_trace_str((s), 1); \
> > + strbuf_addstr((buf), (s)); \
> > + } while (0)
> > +
>
> Likewise for strbuf_addstr().
>
> > void packet_buf_flush(struct strbuf *buf)
> > {
> > - packet_trace("0000", 4, 1);
> > - strbuf_add(buf, "0000", 4);
> > + control_packet_buf_write(buf, "0000");
> > }
Checking the code, it seems like write_str_in_full() and strbuf_addstr()
are both inline defined. I was under the impression that the compiler
will optimise out these strlen()s to be compile-time constants. In fact,
strbuf_addstr() has this comment
NOTE: This function will *always* be implemented as an inline or a macro
using strlen, meaning that this is efficient to write things like:
so if my assumption isn't true, I suspect that we'd need to change the
comment.
