[m-rev.] for review: Fix codegen for subtype field names in high-level data grades.

[Peter Wang]

On Thu, 02 May 2024 01:50:39 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> On 2024-04-30 17:08 +10:00 AEST, "Peter Wang" <novalazy at gmail.com> wrote:
> > diff --git a/compiler/du_type_layout.m b/compiler/du_type_layout.m
> > index 64d7adbc7..f6fb17d69 100644
> > --- a/compiler/du_type_layout.m
> > +++ b/compiler/du_type_layout.m
> > @@ -2,7 +2,7 @@
> >  % vim: ft=mercury ts=4 sw=4 et
> >  %---------------------------------------------------------------------------%
> >  % Copyright (C) 1993-2012 The University of Melbourne.
> > -% Copyright (C) 2015, 2017-2023 The Mercury team.
> > +% Copyright (C) 2015, 2017-2024 The Mercury team.
> >  % This file may only be copied under the terms of the GNU General
> >  % Public License - see the file COPYING in the Mercury distribution.
> >  %---------------------------------------------------------------------------%
> > @@ -725,6 +725,8 @@ check_and_record_du_notag(TypeCtor, Context, Ctors, MaybeCanon,
> >              MaybeDuRepn = have_errors([Spec])
> >          ;
> >              MaybeCanon = canon,
> > +            % We can only get here for a non-subtype du type.
> 
> Do you think it would be helpful to include "non_sub" in the names
> of the predicates that can only be reached from fill_in_non_sub_du_type_repn?
> It would certainly make this code easier to check.

Yes, done.

> 
> > @@ -3781,9 +3802,12 @@ search_ctor_repn_by_unqual_name([CtorRepn | CtorRepns], UnqualName, Arity,
> >  
> >  make_subtype_constructor_arg_repn(CtorArg, BaseCtorArgRepn, CtorArgRepn) :-
> >      CtorArg = ctor_arg(MaybeFieldName, ArgType, Context),
> > -    BaseCtorArgRepn = ctor_arg_repn(_MaybeBaseFieldName, _BaseArgType,
> > -        ArgPosWidth, _BaseContext),
> > -    CtorArgRepn = ctor_arg_repn(MaybeFieldName, ArgType, ArgPosWidth, Context).
> > +    BaseCtorArgRepn = ctor_arg_repn(MaybeBaseFieldName, BaseMaybeBaseCtorArg,
> > +        _BaseArgType, ArgPosWidth, _BaseContext),
> > +    expect(unify(BaseMaybeBaseCtorArg, no_base_ctor_arg), $pred,
> > +        "BaseMaybeBaseCtorArg != no_base_ctor_arg"),
> > +    CtorArgRepn = ctor_arg_repn(MaybeFieldName,
> > +        base_ctor_arg(MaybeBaseFieldName), ArgType, ArgPosWidth, Context).
> 
> I would add a pointer to this predicate in the comment above in the diff (which I deleted)
> which says "this is filled in later".

I've changed that particular comment to match the others which say
"This predicate only deals with non-subtype du types",
along with renaming the predicates.

> > +    % MaybeFieldName is the user-specified field name (if any).
> > +    % MaybeBaseCtorArg tells if this is a field in a subtype, and if so,
> 
> s/tells if/says whether/
> 
> > +    % the field name of the corresponding constructor argument in the base type
> > +    % (if any).
> 
> Move the "(if any)" to just after "the field name".
> 

Done.

> > diff --git a/compiler/ml_unify_gen_util.m b/compiler/ml_unify_gen_util.m
> > index a46b1435d..3dc626682 100644
> > --- a/compiler/ml_unify_gen_util.m
> > +++ b/compiler/ml_unify_gen_util.m
> > @@ -319,7 +319,8 @@ allocate_consecutive_full_word_ctor_arg_repns_boxed(CurOffset,
> >          [Var | Vars], [VarArgRepn | VarArgRepns]) :-
> >      Type = ml_make_boxed_type,
> >      ArgPosWidth = apw_full(arg_only_offset(CurOffset), cell_offset(CurOffset)),
> > -    ArgRepn = ctor_arg_repn(no, Type, ArgPosWidth, dummy_context),
> > +    ArgRepn = ctor_arg_repn(no, no_base_ctor_arg, Type, ArgPosWidth,
> > +        dummy_context),
> >      VarArgRepn = Var - ArgRepn,
> >      allocate_consecutive_full_word_ctor_arg_repns_boxed(CurOffset + 1,
> >          Vars, VarArgRepns).
> 
> I would change the name of this predicate, deleting "_boxed" and replacing it
> with "_for_tuple". That would make clear that no_base_ctor_arg is justified here.
> 
> > @@ -333,7 +334,8 @@ allocate_consecutive_full_word_ctor_arg_repns_lookup(Info, CurOffset,
> >          [Var | Vars], [VarArgRepn | VarArgRepns]) :-
> >      ml_variable_type_direct(Info, Var, Type),
> >      ArgPosWidth = apw_full(arg_only_offset(CurOffset), cell_offset(CurOffset)),
> > -    ArgRepn = ctor_arg_repn(no, Type, ArgPosWidth, dummy_context),
> > +    ArgRepn = ctor_arg_repn(no, no_base_ctor_arg, Type, ArgPosWidth,
> > +        dummy_context),
> >      VarArgRepn = Var - ArgRepn,
> >      allocate_consecutive_full_word_ctor_arg_repns_lookup(Info, CurOffset + 1,
> >          Vars, VarArgRepns).
> 
> I would consider something similar here, but I can't think of an expressive yet short suffix.
> 

I've renamed those two predicates to

    allocate_consecutive_ctor_arg_repns_for_tuple
    allocate_consecutive_ctor_arg_repns_for_extra_args

> > diff --git a/compiler/mlds.m b/compiler/mlds.m
> > index abb799a84..bf7d1da44 100644
> > --- a/compiler/mlds.m
> > +++ b/compiler/mlds.m
> > @@ -2340,7 +2340,8 @@
> >              % When compiling with --high-level-data, we generate a type
> >              % in the target language for each data constructor in a
> >              % discriminated union type. This is the name of one of the fields
> > -            % of this type.
> > +            % of this type. For a subtype, this has to be the name of a field
> > +            % in the base type.
> 
> ... of THE CORRESPONDING field in the base type.
> 
> The diff is otherwise fine.

Thanks for the review.

Peter