Re: [PATCH v3 01/10] pkt-line: extract set_packet_header()

From: Jakub Narębski <jnareb@gmail.com>
Date: Jul 30, 2016, 10:30 UTC
Message-ID: <4081bc44-d964-79ec-165f-f49f33823c17@gmail.com>
In-Reply-To: <20160729233801.82844-2-larsxschneider@gmail.com>

W dniu 30.07.2016 o 01:37, larsxschneider@gmail.com pisze:

Show 5 quoted lines

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> 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!

Show 14 quoted lines

> 
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  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.

Show 11 quoted lines

> -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...

Show 17 quoted lines

> +{
>  	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);
>  }
>  
>