[m-rev.] for review: version_hash_table changes

[Julien Fischer]

Hi Paul,

On Fri, 3 May 2013, Paul Bone wrote:

> For review by anyone.
>
> version_hash_table changes
>
> library/version_hash_table.m:
>    Provide a new from_assoc_list function that gives the caller more
>    control over the new hash table.
>
>    Provide a new function named copy to make a copy of a hash table,
>    this makes it easier for programmers to reason about performance when
>    using version hash tables.

In principle, builtin.copy/2 should do this, but it appears that we
don't yet implement that for version structures.  (See the XXX at the
head of library/version_array.m.)

I suggest that you add a copy function to the hash_table module as well
so as to keep the interfaces to the two modules consistent.


> NEWS:
>    Announce the change.
> ---
> NEWS                         |  3 +++
> library/version_hash_table.m | 35 ++++++++++++++++++++++++++++++++++-
> 2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/NEWS b/NEWS
> index 80789f7..95380c5 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -45,6 +45,9 @@ Changes to the Mercury standard library:
> * We have added predicates to the calendar module for folding over the days
>   in a given range of dates: foldl_days/5, foldl2_days/7 and  foldl3_days/9.
>
> +* We have added two functions to the version_hash_table module:
> +  copy/1, from_assoc_list/4.

    copy/1 and from_assoc_list/4.

...

> diff --git a/library/version_hash_table.m b/library/version_hash_table.m
> index c624101..4e34f39 100644
> --- a/library/version_hash_table.m
> +++ b/library/version_hash_table.m
> @@ -112,6 +112,19 @@
>     %
> :- func num_occupants(version_hash_table(K, V)) = int.
>
> +    % Copy the hash table.
> +    %
> +    % Even though tables can be 'copied' by just creating a second reference
> +    % to the same table this does not always allow programmers to reason
> +    % about performance, as performance depends on how the two references
> +    % are used after that point.
> +    %
> +    % Explicity copying the table with this function makes it easy to see at
> +    % what points in a program the copying operation occurs, making it
> +    % easier to reason about performance.
> +    %

I suggest simply:

        % Make a deep copy of the hash table.

> +:- func copy(version_hash_table(K, V)) = version_hash_table(K, V).
> +
>     % Insert key-value binding into a hash table; if one is
>     % already there then the previous value is overwritten.
>     % A predicate version is also provided.
> @@ -171,7 +184,17 @@
>     %
> :- func to_assoc_list(version_hash_table(K, V)) = assoc_list(K, V).
>
> -    % Convert an association list into a hash table.
> +    % from_assoc_list(HashPred, N, MaxOccupancy, AssocList) = Table,

That line should finish with a colon, not a comma.

> +    % Convert an association list into a hash table.  The first three
> +    % parameters are the same as for init/3 above.
> +    %

> +:- func from_assoc_list(hash_pred(K)::in(hash_pred), int::in, float::in,
> +        assoc_list(K, V)::in) =
> +    (version_hash_table(K, V)::out) is det.
> +
> +    % A simplier version of from_assoc_list/4, the values for N and

s/simplier/simpler/

Cheers,
Julien.