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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Aug 1, 2016, 11:33 UTC
Message-ID: <73AED3FA-C666-4C49-90B2-387E410F7D52@gmail.com>
In-Reply-To: <4081bc44-d964-79ec-165f-f49f33823c17@gmail.com>

Show 24 quoted lines

> On 30 Jul 2016, at 12:30, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> W dniu 30.07.2016 o 01:37, larsxschneider@gmail.com pisze:
>> 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!

Thank you! I would go with this then:

Extract the part of format_packet() that converts an integer to a 4 byte hex string into set_packet_header().

OK?

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

Agreed. Would that be OK?

static void set_packet_header(char *buf, const int size)
{
	static char hexchar[] = "0123456789abcdef";
	#define hex(a) (hexchar[(a) & 15])
	buf[0] = hex(size >> 12);
	buf[1] = hex(size >> 8);
	buf[2] = hex(size >> 4);
	buf[3] = hex(size);
	#undef hex
}

- Lars