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.
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"
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?
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?
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 all.
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.
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?
Sure, I'll do it this weekend. Thanks for the comments.
Created attachment 16231 [details] Clarify docs for cleanup functions Incorporates suggestions in comment 6.
Thanks for the patch. Committed for trunk and 1.2.x. http://svn.apache.org/viewcvs?rev=264750&view=rev