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

On 30 Jul 2016, at 12:30, Jakub Narębski [off-list ref] wrote:

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

quoted

From: Lars Schneider <redacted>

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?

quoted

Signed-off-by: Lars Schneider <redacted>
---
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