Re: [PATCH v2] pkt-line: use string versions of functions

[Denton Liu <liu.denton@gmail.com>]

On Sun, Jun 14, 2020 at 02:35:06PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> 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.