Re: [PATCH v3 01/10] pkt-line: extract set_packet_header()
From: Jakub Narębski <hidden>
Date: 2016-07-30 10:33:33
W dniu 30.07.2016 o 01:37, larsxschneider@gmail.com pisze:
From: Lars Schneider <redacted> set_packet_header() converts an integer to a 4 byte hex string. Make this function locally available so that other pkt-line functions can use it.
This description is not that clear that set_packet_header() is a new function. Perhaps something like the following Extract the part of format_packet() that converts an integer to a 4 byte hex string into set_packet_header(). Make this new function ... I also wonder if the part "Make this [new] function locally available..." is needed; we need to justify exports, but I think we don't need to justify limiting it to a module. If you want to justify that it is "static", perhaps it would be better to say why not to export it. Anyway, I think it is worthy refactoring (and compiler should be able to inline it, so there are no nano-performance considerations). Good work!
quoted hunk ↗ jump to hunk
Signed-off-by: Lars Schneider <redacted> --- pkt-line.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)diff --git a/pkt-line.c b/pkt-line.c index 62fdb37..445b8e1 100644 --- a/pkt-line.c +++ b/pkt-line.c@@ -98,9 +98,17 @@ void packet_buf_flush(struct strbuf *buf) } #define hex(a) (hexchar[(a) & 15])
I guess that this is inherited from the original, but this preprocessor macro is local to the format_header() / set_packet_header() function, and would not work outside it. Therefore I think we should #undef it after set_packet_header(), just in case somebody mistakes it for a generic hex() function. Perhaps even put it inside set_packet_header(), together with #undef. But I might be mistaken... let's check... no, it isn't used outside it.
quoted hunk ↗ jump to hunk
-static void format_packet(struct strbuf *out, const char *fmt, va_list args) +static void set_packet_header(char *buf, const int size) { static char hexchar[] = "0123456789abcdef"; + buf[0] = hex(size >> 12); + buf[1] = hex(size >> 8); + buf[2] = hex(size >> 4); + buf[3] = hex(size); +} + +static void format_packet(struct strbuf *out, const char *fmt, va_list args)
It is strange how 'git diff' chosen to represent this patch...
quoted hunk ↗ jump to hunk
+{ size_t orig_len, n; orig_len = out->len;@@ -111,10 +119,7 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args) if (n > LARGE_PACKET_MAX) die("protocol error: impossibly long line"); - out->buf[orig_len + 0] = hex(n >> 12); - out->buf[orig_len + 1] = hex(n >> 8); - out->buf[orig_len + 2] = hex(n >> 4); - out->buf[orig_len + 3] = hex(n); + set_packet_header(&out->buf[orig_len], n); packet_trace(out->buf + orig_len + 4, n - 4, 1); }