This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 4/4] Consolidate posix_fadvise implementations
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.