W dniu 30.07.2016 o 01:37, larsxschneider@xxxxxxxxx pisze:
> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>
> 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!
>
> Signed-off-by: Lars Schneider <larsxschneider@xxxxxxxxx>
> ---
> 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.
> -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...
> +{
> 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);
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html