[m-rev.] for review: lookup_name_default_prefix

Julien Fischer jfischer at opturion.com
Mon May 29 18:53:37 AEST 2023


On Mon, 29 May 2023, Zoltan Somogyi wrote:

> Add lookup_name_default_prefix to varset.m.
> 
> library/varset.m:
>     We have long had two versions of the lookup_name function, and of
>     the equivalent lookup_name predicate, with the difference being
>     that one version had an extra argument that specified to prefix
>     for the names to be constructed for not-explicitly-named variables.
>
>     Obsolete the function and the predicate with this extra argument
>     in favor of a new function/predicate pair, whose shared name is
>     lookup_name_default_prefix.
>
>     Act on an old sortof-XXX by changing the order of arguments of the
>     set_bindings predicate to make it state-variable-friendly.

...

> diff --git a/NEWS.md b/NEWS.md
> index 1701e14d5..59d5b77be 100644
> --- a/NEWS.md
> +++ b/NEWS.md
> @@ -1800,10 +1800,21 @@ Changes to the Mercury standard library
>
>  * The following functions and predicates have been added:
> 
> +    - func `lookup_name_default_prefix/3`
> +    - pred `lookup_name_default_prefix/4`
>      - func `unname_var/2`
>      - pred `unname_var/3`
>      - pred `undo_default_names/2`
> 
> +* The following functions and predicates have been marked obsolete:

function and predicate

> +    - func `lookup_name/3`      (replacement: `lookup_name_default_prefix/3`)
> +    - pred `lookup_name/4`      (replacement: `lookup_name_default_prefix/4`)
> +

...

> diff --git a/library/term.m b/library/term.m
> index 3800694fd..bc23fb1f6 100644
> --- a/library/term.m
> +++ b/library/term.m
> @@ -117,10 +117,11 @@
>
>  %---------------------------------------------------------------------------%
> 
> -    % from_int/1 should only be applied to integers returned by to_int/1.
> -    % NOTE_TO_IMPLEMENTORS This instance declaration is needed to allow
> -    % NOTE_TO_IMPLEMENTORS sets of variables to be represented using
> -    % NOTE_TO_IMPLEMENTORS sparse_bitset.m and the other bitset modules.
> +    % from_int/1 should only be applied to integers returned by to_int/1, and
> +    % from_uint/1 should only be applied to integers returned by to_uint/1.
> +    % NOTE_TO_IMPLEMENTORS The uenum instance declaration is needed to allow
> +    % note_to_implementors sets of variables to be represented using
> +    % note_to_implementors sparse_bitset.m and the other bitset modules.

You've lowercased note_to_implementors on the last two lines there.

>  :- instance enum(var(_)).
>  :- instance uenum(var(_)).

> diff --git a/library/varset.m b/library/varset.m
> index 28ca34894..174984708 100644
> --- a/library/varset.m
> +++ b/library/varset.m
> @@ -10,10 +10,10 @@
>  % Main author: fjh.
>  % Stability: low.
>  %
> -% This file provides facilities for manipulating collections of variables.
> -% through the 'varset' ADT. These variables are object-level variables,
> -% and are represented as ground terms, so it might help to think of them
> -% as "variable ids" rather than Prolog-style variables.
> +% This file provides facilities for manipulating collections of variables,
> +% through the 'varset' abstract data type. These variables are object-level
> +% variables, and are represented as ground terms, so the right way to think
> +% of them is as "variable ids" rather than Prolog-style variables.
>  %
>  % A varset may record a name and/or a value (binding) with each variable.
>  %
> @@ -31,9 +31,11 @@
>  % In situations in which this is not a concern, programmers may use
>  % the standard generic varset instance.
>  %
> -% Note that there are some design flaws in the relationship between
> -% varset.m and term.m. There is too much coupling between the two,
> -% which may and should be fixed later, e.g. by merging the two modules.
> +% Note that varset.m and term.m are strongly coupled together, meaning that
> +% they each need the other. The reason why they have not been merged into
> +% one larger module is that many user modules call predicates and functions
> +% from just one of these two modules, even though, through that one module,
> +% they implicily depend on the other as well.

s/implicily/implicitly/

That looks fine otherwise.

Julien.


More information about the reviews mailing list