Re: [PATCH v2 5/5] convert: add filter.<driver>.process option

On 29 Jul 2016, at 13:24, Jakub Narębski [off-list ref] wrote:

W dniu 2016-07-29 o 12:38, Lars Schneider pisze:

quoted

On 27 Jul 2016, at 11:41, Eric Wong [off-list ref] wrote:

quoted

larsxschneider@gmail.com wrote:

quoted

quoted

quoted

+static off_t multi_packet_read(struct strbuf *sb, const int fd, const size_t size)

I'm no expert in C, but this might be const-correctness taken
too far.  I think basing this on the read(2) prototype is less
surprising:

 static ssize_t multi_packet_read(int fd, struct strbuf *sb, size_t size)

Hm... ok. I like `const` because I think it is usually easier to read/understand
functions that do not change their input variables. This way I can communicate
my intention to future people modifying this function!

Well, scalar types like `size_t` are always passed by value, so here `const`
doesn't matter, and it makes line longer.  I think library functions do not
use `const` for `size_t` parameters.

You are reading from the file descriptor `fd`, so it state would change.
Using `const` feels a bit like lying.  Also, it is scalar type.

OK, since you are the second reviewer arguing against `const` I will remove it.

[...] 

quoted

I agree with your reordering of the parameters, though!

Speaking of coding style... convert.c is already big and gets only bigger 
with this patch (1720 lines). Would it make sense to add a new file 
"convert-pipe-protocol.c"
or something for my additions?

I wonder if it would be possible to enhance existing functions, instead
of redoing them (at least in part) for per-command filter driver protocol.

I think I reused as much as possible.

Thanks,
Lars