Re: [PATCH 4/4] Consolidate posix_fadvise implementations

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 23/08/2016 17:00, Yury Norov wrote:
> On Tue, Aug 23, 2016 at 04:29:24PM -0300, Adhemerval Zanella wrote:
>>>> +#ifndef __OFF_T_MATCHES_OFF64_T
>>>> +
>>>> +/* Both arm and powerpc implements fadvise64_64 with last 'advise' argument
>>>> +   just after 'fd' to avoid the requirement of implementing 7-arg syscalls.
>>>> +   ARM also defines __NR_fadvise64_64 as __NR_arm_fadvise64_64.
>>>> +
>>>> +   tile requires __ASSUME_ALIGNED_REGISTER_PAIRS but implements the 32-bit
>>>> +   fadvise64_64 without the padding 0 after fd.
>>>> +
>>>> +   s390 implements fadvice64_64 using a specific struct with arguments
>>>> +   packed inside.  This is the only implementation handled in arch-specific
>>>> +   code.  */
>>>
>>> Are you sure it will be the only implementation forever, and comment
>>> will be valid too?
>>
>> The comment is meant to only describe supported architectures at the
>> time the patch was created.  Any other port that either deviates
>> from current approach or uses a already describe one should update
>> this description.
>  
> And I bet, one will forget to do so. That's why I don't like this sort of
> comments. It assumes that someone (you) will constantly monitor the patch
> traffic and check all the patches against this comment. This assumption is
> wrong of course. So the comment will come broken one day, and will confuse 
> people instead of helping them. That's why people don't read comments.
> 
> With or without it, I'll use grep to find all implementations of this
> function.

As for any code comment, this is a best effort to describe the current
situation for any potential reader. It is not meant to be code
documentation, but rather an advice on why I decided to add the two
constants (__ASSUME_FADVISE64_64_6ARG, __ASSUME_FADVISE64_64_NO_ALIGN),
why I think s390 is an outlier that still should have a specific 
implementation, and why ARM and MIPS64 still have specific files.

I still see them as valuable for someone that might check why the
current code is organized in the way it is.

> 
> You touch arm and tile code, and you explain what you do. But you
> don't touch s390, and you _can_ avoid mentioning it here. I see 3
> options:
>  - move the comment that s390 is the only exception now to the patch
>    description (preferable for me).
>  - add date to the comment. It's fun but doesn't work much.
>  - generalize s390 too.

I think commenting s390-32 outlier in patch description should be enough.
I see little point of generalizing s390 now, however if a future port 
decide to use the same strategy I think it might worth it.

> 
> To be serious, the benefit of removing all custom implementations of
> fadvise() belongs to your patch, not to the function fadvise() itself.
> And it should be underlined at right place.