> On 30 Jul 2016, at 12:30, Jakub Narębski <jnareb@xxxxxxxxx> wrote:
>
> 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!
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?
>>
>> 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.
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--
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