[m-rev.] for review: fix bug #230

[Julien Fischer]

Hi,

Thanks for looking at this so promptly.

On Tue, 22 Nov 2011, Peter Wang wrote:

> Julien, I assume this fix would be too intrusive for 11.07, so you
> should go ahead with disabling the direct arg functor optimisation there.

Done - I will have a look at merging the changes on to the 11.07 branch
*after* the release; we might re-enable direct arg functors in 11.07.

> Branches: main
>
> Fix bug #230.  When compiling a sub-module we could incorrectly use the
> direct argument functor representation for a d.u. functor when:
>
>    1. the outer type is defined and exported from an ancestor module, and
>    2. the functor argument type is defined in the same ancestor module,
>       but NOT exported.

That's not clear; the functor argument type is (abstract) exported; it's
the definition of the functor argument type that is not exported.

> e.g.
>
>    :- module ancestor.
>    :- interface.
>    :- type outer ---> f(inner) ; ... .

There should be

    :- type inner.

there too, otherwise that example shouldn't compile.

>    :- implementation.
>    :- type inner ---> inner( ... ).

> To solve the problem, the sub-module must be able to distinguish between type
> definitions which are available from the ancestor module's public interface,
> and type definitions which the ancestor only exported to sub-modules.
> The distinction was not apparent in the `.int0' private interface files --
> all items were contained into a single interface section, no matter
> where they came from in the original source file.
>
> This change separates `.int0' files into two sections: the interface section
> for items in the public interface, and the implementation section for items in
> the private interface.
>
> compiler/modules.m:
>        Change `.int0' files to have two sections as described.
>
>        Sort the contents of the individual sections.
>
> compiler/add_type.m:
>        Do not let other statuses override
>        `status_imported(import_locn_ancestor_private_interface_proper)'.
>
> compiler/make_tags.m:
>        Fix the main bug described.
>
>        Fix a bug where a type with `status_abstract_exported' was not
>        considered exported-to-submodules, which it is.
>
> compiler/prog_data.m:
>        Rename and clarify that `import_locn_ancestor_private_interface_proper'
>        really, really means that it came from the actual, true, and proper
>        private interface of a module and nowhere else.
>
> compiler/prog_item.m:
>        Rename `md_private_interface' to what it really means.
>
> compiler/equiv_type.m:
> compiler/hlds_out_pred.m:
> compiler/hlds_pred.m:
> compiler/make_hlds_passes.m:
> compiler/mercury_to_mercury.m:
> compiler/module_qual.m:
>        Conform to changes.
>
> tests/hard_coded/Mmakefile:
> tests/hard_coded/daf_bug.exp:
> tests/hard_coded/daf_bug.m:
> tests/hard_coded/daf_bug_parent.m:
> tests/hard_coded/daf_bug_sub.m:
>        Add test case.
>
> tests/invalid/ii_parent.ii_child.err_exp:
>        Update expected output.

...

> diff --git a/compiler/hlds_out_pred.m b/compiler/hlds_out_pred.m
> index 75c3f04..288ca69 100644
> --- a/compiler/hlds_out_pred.m
> +++ b/compiler/hlds_out_pred.m
> @@ -498,7 +498,7 @@ import_status_to_string(status_imported(import_locn_interface)) =
> import_status_to_string(status_imported(import_locn_implementation)) =
>     "imported in the implementation".
> import_status_to_string(status_imported(
> -        import_locn_ancestor_private_interface)) =
> +        import_locn_ancestor_private_interface_proper)) =
>     "imported from an ancestor's private interface".
> import_status_to_string(status_imported(import_locn_ancestor)) =
>     "imported by an ancestor".
> diff --git a/compiler/hlds_pred.m b/compiler/hlds_pred.m
> index 1d533fc..ffec4fc 100644
> --- a/compiler/hlds_pred.m
> +++ b/compiler/hlds_pred.m
> @@ -250,7 +250,8 @@
>                 % due to its presence in the .opt files
>                 % (intermod.adjust_pred_import_status).
>     ;       status_abstract_exported
> -                % Describes a type with only an abstract declaration exported.
> +                % Describes a type with only an abstract declaration exported
> +                % to non-sub-modules. It *is* exported to sub-modules.

I would reword the last sentence to say:

     The definition of the type is exported to sub-modules.

>     ;       status_pseudo_exported
>                 % The converse of pseudo_imported; this means that only the
>                 % (in, in) mode of a unification is exported.

...

> diff --git a/compiler/modules.m b/compiler/modules.m
> index 679ba68..fc62b30 100644
> --- a/compiler/modules.m
> +++ b/compiler/modules.m

...

> @@ -1540,22 +1551,78 @@ clause_in_interface_warning(ClauseOrPragma, Context) = Spec :-
>     Spec = error_spec(severity_warning, phase_term_to_parse_tree,
>         [simple_msg(Context, [always(Pieces)])]).
>
> -    % strip_clauses_from_interface is the same as
> -    % check_for_clauses_in_interface except that it doesn't issue any
> -    % warnings, and that it also strips out the `:- interface' and `:-
> -    % implementation' declarations.
> +    % strip_clauses_private_interface is used when creating the private
> +    % interface (`.int0') files for packages with sub-modules. It removes
> +    % unnecessary items and separates interface and implementation items.
> +    %
> +    % The `.int0' file contains items which is available to any module in the

which *are* available ...

> +    % interface section, and items which are only available to sub-modules in
> +    % the implementation section. The term "private interface" is ambiguous:
> +    % sometimes it refers to the `.int0' file which, as just explained,
> +    % contains the public interface as well. The term "private interface
> +    % proper" may be used to refer to the information in the implementation
> +    % section of the `.int0' file.
>     %
> -    % This is used when creating the private interface (`.int0') files for
> -    % packages with sub-modules.
> +    % (Historically, the `.int0' file did not distinguish between the public
> +    % and private interfaces.)

That looks fine otherwise.

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------