[m-rev.] for review: Add foreign type assertion `word_aligned_pointer'.

[Zoltan Somogyi]

On Wed, 30 Sep 2015 10:46:48 +1000, Peter Wang <novalazy at gmail.com> wrote:
> [Feel free to suggest an alternative name for the assertion.]

I think the name you picked is fine.

> diff --git a/NEWS b/NEWS
> index 0431759..823b465 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -23,6 +23,8 @@ Changes to the Mercury language:
>  * We have added an extension to include external files
>    in pragma foreign_decl and pragma foreign_code declarations.
>  
> +* We have added a foreign type assertion `word_aligned_pointer'.
> +

I think an additional sentence saying *why* this is useful would be in order.

> +    % At this point we have checked that the constructor has a single argument,
> +    % and the argument has a suitable type such that the direct arg functor
> +    % representation may apply to the constructor. We still need to check that
> +    % other modules would infer the same type representation for the same
> +    % constructor, given that they may not have the same knowledge of the
> +    % constructor's type or the argument type.
> +    %
> +    % TypeStatus is the import status of the type of the constructor being
> +    % checked.
> +    %
>  :- pred check_direct_arg_cond(type_status::in, direct_arg_cond::in) is semidet.

Reword "At this point ..." along the lines of "When this predicate is called",
and state what *should* have been done then, not what *has* been done, since
the latter can change. I would also give the predicate a more meaningful name.

> +:- pred is_foreign_type_for_target(hlds_type_body::in, compilation_target::in,
> +    foreign_type_assertions::out) is semidet.
> +
> +is_foreign_type_for_target(TypeBody, Target, Assertions) :-
> +    (
> +        TypeBody ^ du_type_is_foreign_type = yes(ForeignType)
> +    ;
> +        TypeBody = hlds_foreign_type(ForeignType)
> +    ),

Replace the semidet field access with a unification that makes it clear that
this is a switch, not a disjunction.

> diff --git a/compiler/prog_data.m b/compiler/prog_data.m
> index ef49ba3..5ae6a6c 100644
> --- a/compiler/prog_data.m
> +++ b/compiler/prog_data.m
> @@ -1650,7 +1650,7 @@ cons_id_is_const_struct(ConsId, ConstNum) :-
>      ;       parse_tree_foreign_type(
>                  foreign_lang_type   :: foreign_language_type,
>                  foreign_user_uc     :: maybe(unify_compare),
> -                foreign_assertions  :: list(foreign_type_assertion)
> +                foreign_assertions  :: foreign_type_assertions
>              ).
>  
>  :- type abstract_type_details
> @@ -1674,9 +1674,13 @@ cons_id_is_const_struct(ConsId, ConstNum) :-
>              % (i.e. the type was declared with
>              % `:- solver type ...').
>  
> +:- type foreign_type_assertions
> +    --->    foreign_type_assertions(list(foreign_type_assertion)).
> +

Is there any reason why the argument of foreign_type_assertions is not a set?

> diff --git a/tests/hard_coded/word_aligned_pointer_2.m b/tests/hard_coded/word_aligned_pointer_2.m
> @@ -0,0 +1,82 @@
> +%---------------------------------------------------------------------------%
> +
> +:- pragma foreign_proc("C",
> +    get_foo(X::in) = (Int::out),
> +    [will_not_call_mercury, promise_pure, thread_safe],
> +"
> +    assert(X == MR_strip_tag(X));
> +    Int = MR_unmkbody(X);
> +").

In C, assert() can be defined to be (and is often defined to be) a no-op,
so write explicit code to abort the program if the test fails.

Other than that, the diff is fine.

Zoltan.