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

[Lars Schneider <larsxschneider@gmail.com>]

> On 03 Aug 2016, at 22:05, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> [This response might have been invalidated by v4]
> 
> W dniu 01.08.2016 o 13:33, Lars Schneider pisze: 
>>> On 30 Jul 2016, at 12:30, Jakub Narębski <jnareb@gmail.com> wrote:
> 
>>>> #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
>> }
> 
> That's better, though I wonder if we need to start #defines at begining
> of line.  But I think current proposal is O.K.
> 
> 
> Either this (which has unnecessary larger scope)
> 
>  #define hex(a) (hexchar[(a) & 15])
>  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);
>  }
>  #undef hex
> 
> or this (which looks worse)
> 
>  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
>  }
> 

I probably will drop this patch as Junio is not convinced that it
is a good idea:
http://public-inbox.org/git/xmqqd1lp8v2o.fsf%40gitster.mtv.corp.google.com/

- Lars