Bug 35975

Summary: Confusing documentation for apr_pool_cleanup_run()
Product: APR Reporter: Jonathan Wakely <zilla>
Component: APRAssignee: Apache Portable Runtime bugs mailinglist <bugs>
Severity: normal Keywords: PatchAvailable
Priority: P2    
Version: HEAD   
Target Milestone: ---   
Hardware: Other   
OS: other   
Attachments: Fix confusing docs
Clarify docs for cleanup functions
Clarify docs for cleanup functions

Description Jonathan Wakely 2005-08-02 18:34:08 UTC
The docs for apr_pool_cleanup_run() say the function will "Use 'data' instead of
the data that was registered with the cleanup."

See http://apr.apache.org/docs/apr/group__apr__pools.html#ga27

This is confusing and wrong, because unless the same address is passed as was
given when registering the cleanup then no cleanup will be killed, so there is
no "data that was registered with the cleanup".  Alternatively, if the same
pointer was given and a cleanup killed, then the two 'data' pointers have the
same value, so what does it matter which one is used?

The comment seems to have been written by someone trying to describe the
implementation, which is completely irrelevant to a user of the function who
only wants to know how it works.  The part about which 'data' is used should be
removed from the docs.
Comment 1 Jonathan Wakely 2005-08-02 18:36:06 UTC
Created attachment 15853 [details]
Fix confusing docs

This patch removes the part I think is confusing, and corrects two instances of
"The pool register the cleanup with" changing it to "The pool to register the
cleanup with"
Comment 2 Jonathan Wakely 2005-08-02 18:38:27 UTC
I feel a need to go through the docs for the various apr_pool_cleanup_xxx()
functions and add more descriptive docs,  explaining that the combination of
function pointer, data and pool defines "a cleanup" and that the same
combination must be used to unregister a cleanup.  Before I waste my time, would
such a patch be accepted?
Comment 3 William A. Rowe Jr. 2005-08-02 18:45:26 UTC
  We absolutely entertain docs patches, as well as code patches.  Please feel
  free to proceed.  Should we keep this incident open and commit your entire
  set of docs patches at once?
Comment 4 Jonathan Wakely 2005-08-02 18:50:30 UTC
Thanks for the quick response.  I'll try to submit a further patch today or
tomorrow, so if you want to wait for that then I'll use this bug to collect them
Comment 5 Jonathan Wakely 2005-08-11 21:56:25 UTC
Created attachment 16014 [details]
Clarify docs for cleanup functions

(Sorry for the delay, I was on holiday.)

This patch adds a Doxygen group for the cleanup functions, fixes some typos and
IMHO clarifies the behaviour of some of the cleanup functions.

For the functions that unregister a cleanup I have said:

  The most recently registered cleanup with the same values of @a p,
  @a data and @a cleanup will be removed.

That description doesn't explicitly say that the cleanup is registered with
pool 'p' but I think saying "the same values of p, data and cleanup" is
accurate enough but also short and pithy.  Would it be better to say something
like this?

  Removes the last cleanup registered with @a p with the same values
  of @a data and @cleanup.

I think the wording I chose is most easily understood by a quick glance at the
docs and/or by people for whom English is not their first language, but I'm
open to better suggestions.
Comment 6 Joe Orton 2005-08-24 16:54:52 UTC
Thanks for the patch.  This:

+ * The most recently registered cleanup with the same values of @a p,
+ * @a data and @a cleanup will be removed

seems slightly ambiguous, as if there is some global list of cleanups indexed by
(p, data, cleanup); I'd suggest instead:

 * The most cleanup most recently registered with p having the same values of
 * @a data and @a cleanup will be removed

it would also worth noting that apr_pool_cleanup_for_exec runs all cleanups
registered with the global pool and all children thereof.  Any chance you could
clean these these two things up and resubmit the patch to dev@apr.apache.org?
Comment 7 Jonathan Wakely 2005-08-26 13:49:18 UTC
Sure, I'll do it this weekend. Thanks for the comments.
Comment 8 Jonathan Wakely 2005-08-28 12:03:31 UTC
Created attachment 16231 [details]
Clarify docs for cleanup functions

Incorporates suggestions in comment 6.
Comment 9 Joe Orton 2005-08-30 12:40:19 UTC
Thanks for the patch.  Committed for trunk and 1.2.x.