| Message ID | da9ba5fb2d0fb9481f81ce525cbabaedd722858d.1592125442.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] pkt-line: use string versions of functions | expand |
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"); > }
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.
On 2020-06-14 05:07:54-0400, Denton Liu <liu.denton@gmail.com> wrote: > Hi Đoàn, Sorry for this late reply, I have another stuff to work on. > Perhaps something like this? This looks better, but ... > > pkt-line.c | 48 ++++++++++++++++++++++++++++++------------------ > 1 file changed, 30 insertions(+), 18 deletions(-) > > diff --git a/pkt-line.c b/pkt-line.c > index 8f9bc68ee2..022376f00f 100644 > --- a/pkt-line.c > +++ b/pkt-line.c > @@ -81,49 +81,61 @@ static void packet_trace(const char *buf, unsigned int len, int write) > strbuf_release(&out); > } > > +static inline void packet_trace_str(const char *buf, int write) > +{ > + packet_trace(buf, strlen(buf), write); > +} > + > +#define control_packet_write(fd, s, errstr) \ > + do { \ > + (void)s"is a string constant"; \ If we are going to not wrap s inside () here, > + packet_trace_str((s), 1); \ I see no point in wrapping it here. And wrapping around "string" is not standard C, some (or most) compilers support it as an extension. See USE_PARENS_AROUND_GETTEXT_N Can we just put it straight? packet_trace(s, strlen(s), 1); > + if (write_str_in_full((fd), (s)) < 0) \ > + die_errno((errstr)); \ > + } while (0) > +
diff --git a/pkt-line.c b/pkt-line.c index 8f9bc68ee2..022376f00f 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -81,49 +81,61 @@ static void packet_trace(const char *buf, unsigned int len, int write) strbuf_release(&out); } +static inline void packet_trace_str(const char *buf, int write) +{ + packet_trace(buf, strlen(buf), write); +} + +#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) + /* * 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")); } void packet_delim(int fd) { - packet_trace("0001", 4, 1); - if (write_in_full(fd, "0001", 4) < 0) - die_errno(_("unable to write delim packet")); + control_packet_write(fd, "0001", _("unable to write delim packet")); } void packet_response_end(int fd) { - packet_trace("0002", 4, 1); - if (write_in_full(fd, "0002", 4) < 0) - die_errno(_("unable to write stateless separator packet")); + control_packet_write(fd, "0002", _("unable to write stateless separator packet")); } int packet_flush_gently(int fd) { - packet_trace("0000", 4, 1); - if (write_in_full(fd, "0000", 4) < 0) + packet_trace_str("0000", 1); + if (write_str_in_full(fd, "0000") < 0) return error(_("flush packet write failed")); return 0; } +#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) + void packet_buf_flush(struct strbuf *buf) { - packet_trace("0000", 4, 1); - strbuf_add(buf, "0000", 4); + control_packet_buf_write(buf, "0000"); } void packet_buf_delim(struct strbuf *buf) { - packet_trace("0001", 4, 1); - strbuf_add(buf, "0001", 4); + control_packet_buf_write(buf, "0001"); } void set_packet_header(char *buf, int size) @@ -337,15 +349,15 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer, if (len < 0) { die(_("protocol error: bad line length character: %.4s"), linelen); } else if (!len) { - packet_trace("0000", 4, 0); + packet_trace_str("0000", 0); *pktlen = 0; return PACKET_READ_FLUSH; } else if (len == 1) { - packet_trace("0001", 4, 0); + packet_trace_str("0001", 0); *pktlen = 0; return PACKET_READ_DELIM; } else if (len == 2) { - packet_trace("0002", 4, 0); + packet_trace_str("0002", 0); *pktlen = 0; return PACKET_READ_RESPONSE_END; } else if (len < 4) {
We have many cases where we are writing a control packet as a string constant out and we need to specify the length of the string. Currently, the length is specified as a magical `4` literal. Change these instances to use a function that calls strlen() to determine the length of the string removing the need to specify the length at all. Since these functions are inline, the strlen()s should be replaced with constants at compile-time so this should not result in any performance penalty. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Hi Đoàn, Perhaps something like this? pkt-line.c | 48 ++++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-)