Re: [PATCH] Add and use new glibc-internal futex API in sparc code.

[Roland McGrath <roland at hack dot frob dot com>]

> On Tue, 2015-06-09 at 13:51 -0700, Roland McGrath wrote:
> > Arguably every use of ignore_value should have a comment explaining why
> > it's safe to elide the error checking.
> 
> That's always the same pattern.  Would it be okay for you if I just note
> that pattern in the futex_wait comments?  

That is certainly good to do, but I think we still want at least a brief
comment for every use of ignore_value where it's not totally obvious (most
existing uses are in stubs where it's clear).  A detailed comment about the
general case in one place means that the individual comments can just be 
very terse like /* futex_waits errors ignored since we'll retry.  */

But if it's a commonly-repeated case, then it probably merits a wrapper
macro that constitutes self-documentation.  i.e., define a
"futex_wait_ignore_error" or "futex_wait_noerror" macro to use in all these
places, and then the comment on the macro definition can explain where it's
appropriate to use and why.  Or perhaps name the macro "futex_wait_in_loop"
or something better that more obviously says in the name where it's
appropriate to use rather than just what is concretely does.

The upshot is that avoiding error checking is always unusual and a casual
reader of just a little bit of code (especially a reviewer seeing just the
patch context) should not have to hunt around to convince himself that it
is safe.  If it's a macro that makes its uses self-documenting, then that
does it.  If it's a terse comment that points clearly to where fuller
rationale can be read, then that does it too.


Thanks,
Roland