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

From: Jakub Narębski <jnareb@gmail.com>
Date: Jul 29, 2016, 11:24 UTC
Message-ID: <579B3CFF.6040601@gmail.com>
In-Reply-To: <64C7D52F-9030-460C-8F61-4076F5C1DDF6@gmail.com>

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

> On 27 Jul 2016, at 11:41, Eric Wong <e@80x24.org> wrote:
>> larsxschneider@gmail.com wrote:

Show 11 quoted lines

>>> +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.
 
[...]

Show 6 quoted lines

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

Best,

-- 
Jakub Narębski