git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Denton Liu <liu.denton@gmail.com>
Date
Jun 14, 2020, 22:28 UTC
Message-ID
<20200614222828.GA4846@generichostname>
In-Reply-To
<xmqqeeqhxred.fsf@gitster.c.googlers.com>
On Sun, Jun 14, 2020 at 02:35:06PM -0700, Junio C Hamano wrote:
Show 43 quoted lines
> 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.

Previous: Junio C HamanoNext: Đoàn Trần Công Danh
Message 11 of 22 in “pkt-line: extract out PACKET_HEADER_SIZE”
  1. pkt-line: extract out PACKET_HEADER_SIZEDenton Liu, Jun 13, 2020
  2. Đoàn Trần Công DanhJun 13, 2020
  3. Denton LiuJun 13, 2020
  4. Junio C HamanoJun 13, 2020
  5. Junio C HamanoJun 14, 2020
  6. 0/3 pkt-line: war on magical `4` literalDenton Liu, Jun 14, 2020
  7. 2/3 pkt-line: use string versions of functionsDenton Liu, Jun 14, 2020
  8. Đoàn Trần Công DanhJun 14, 2020
  9. pkt-line: use string versions of functionsDenton Liu, Jun 14, 2020
  10. Junio C HamanoJun 14, 2020
  11. Denton LiuJun 14, 2020
  12. Đoàn Trần Công DanhJun 15, 2020
  13. 1/3 remote-curl: use strlen() instead of magic numbersDenton Liu, Jun 14, 2020
  14. 3/3 pkt-line: extract out PACKET_HEADER_SIZEDenton Liu, Jun 14, 2020
  15. Junio C HamanoJun 14, 2020
  16. 0/3 pkt-line: war on magical `4` literalDenton Liu, Jun 23, 2020
  17. 1/3 remote-curl: use strlen() instead of magic numbersDenton Liu, Jun 23, 2020
  18. Jeff KingJun 23, 2020
  19. Junio C HamanoJun 23, 2020
  20. 2/3 pkt-line: use string versions of functionsDenton Liu, Jun 23, 2020
  21. Jeff KingJun 23, 2020
  22. 3/3 pkt-line: extract out PACKET_HEADER_SIZEDenton Liu, Jun 23, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.