On 22.12.2015 22:09, William A Rowe Jr wrote:
Same question again, is this an apr or svn behavior change?
Sorry, I must have missed your initial reply then. APR's
implementation grew more capable over the last years.
A change in SVN made us aware that earlier APR releases
were less capable. None of that needs to be changed.

The best way to characterize this patch submission
is as a "request for APR API spec update".

SVN code recently encountered the "both hashes must
use the same function" restriction of apr_hash_overlay.
Technically, everything is fine because the docs clearly
say what you can and cannot do and users can work
around it (as we have).

As it turned out, the implementation no longer requires
the restriction on the hash functions used. Even if it
still did, it would be very easy to change the APR code
to no longer require the restriction (as done in 1.4.6+).

Now, it is a perfectly valid position to say "we don't want
to extend our API guarantees", potentially allowing for
clever implementations in the future. In this particular
case, however, I don't see the benefit of sticking with
the more restrictive spec. It only burdens the user with
a workaround they may not realize is necessity and that
will have no impact on test results.

APIs should be easy to use correctly and hard to use
incorrectly. Therefore, I strongly suggest the spec update.

-- Stefan^2.

On Mon, Dec 21, 2015 at 4:45 PM, Stefan Fuhrmann <[email protected] <mailto:[email protected]>> wrote:

    On 11.12.2015 11:32, Bert Huijben wrote:



            -----Original Message-----
            From: Stefan Fuhrmann [mailto:[email protected]
            <mailto:[email protected]>]
            Sent: donderdag 10 december 2015 23:59
            To: [email protected] <mailto:[email protected]>
            Subject: [PATCH] APR hash documentation

            Hi there,

            since 1.4.6, the hash implementation allows for
            mismatching and custom hash functions when merging
            hashes.  But the docstrings have not been updated.


        I don't think this should be applied without leaving a note
        about not having
        that support in earlier versions.

        The reason we found this problem is that we tested with those
        older
        versions. If we remove that part of the documentation it will
        be even harder
        to diagnose problems like this in the future.


    Fair enough. Here the updated version against /trunk.

    -- Stefan^2.

    [[[
    Since 1.4.6, apr_hash_overlay and apr_hash_merge work with any
    combination
    of hash functions.  Reflect that change in the documentation.

    * include/apr_hash.h
       (apr_hash_overlay,
        apr_hash_merge): Update the docstrings.
    ]]]

    [[[
    Index: include/apr_hash.h
    ===================================================================
    --- include/apr_hash.h  (revision 1721274)
    +++ include/apr_hash.h  (working copy)
    @@ -216,8 +216,8 @@

     /**
      * Merge two hash tables into one new hash table. The values of
    the overlay
    - * hash override the values of the base if both have the same
    key.  Both
    - * hash tables must use the same hash function.
    + * hash override the values of the base if both have the same
    key.  Prior
    + * to release 1.4.6, both hash tables had to use the same hash
    function.
      * @param p The pool to use for the new hash table
      * @param overlay The table to add to the initial table
      * @param base The table that represents the initial values of
    the new table
    @@ -230,8 +230,8 @@
     /**
      * Merge two hash tables into one new hash table. If the same key
      * is present in both tables, call the supplied merge function to
    - * produce a merged value for the key in the new table. Both
    - * hash tables must use the same hash function.
    + * produce a merged value for the key in the new table. Prior to
    + * release 1.4.6, both hash tables had to use the same hash
    function.

      * @param p The pool to use for the new hash table
      * @param h1 The first of the tables to merge
      * @param h2 The second of the tables to merge
    ]]]



Reply via email to