This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 4/4] Consolidate posix_fadvise implementations


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.

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.

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]