[m-rev.] for review: do not hardcode the list of builtin primtive types in so many places
jfischer at opturion.com
Mon Apr 10 09:46:22 AEST 2017
On Mon, 10 Apr 2017, Zoltan Somogyi wrote:
>> On 10 Apr 2017, at 00:50, Julien Fischer <jfischer at opturion.com> wrote:
>> Add a comment document other places in the compiler that will need
>> to be updated if a new primitive type is added.
> A word is missing here.
>> Use the new predicates rather than listing the primitive types
> The key to the correctness of this change is not the modules that
> have been updated to use the new predicates, but the modules that haven’t,
> even though they should be. May I presume that you have looked at all
> the modules in the compiler that mention all of “character”, “string”,
> “int” and “float”?
>> --- a/compiler/add_foreign_enum.m
>> +++ b/compiler/add_foreign_enum.m
>> @@ -55,12 +55,7 @@ add_pragma_foreign_export_enum(FEEInfo, _TypeStatus, Context,
>> % Emit an error message for foreign_export_enum pragmas for the
>> % builtin atomic types.
>> TypeArity = 0,
>> - ( TypeName = unqualified("character")
>> - ; TypeName = unqualified("float")
>> - ; TypeName = unqualified("uint")
>> - ; TypeName = unqualified("int")
>> - ; TypeName = unqualified("string")
>> - )
>> + is_builtin_type_sym_name(TypeName)
> Indentation seems to be off, both here and below.
It's fine in the actual file.
>> index 14b0c9a..eeb14d6 100644
>> --- a/compiler/prog_data.m
>> +++ b/compiler/prog_data.m
>> @@ -482,6 +482,16 @@ cons_id_is_const_struct(ConsId, ConstNum) :-
>> % A type expression with an explicit kind annotation.
>> % (These are not yet used.)
>> + % This type enumerates all of the builtin primitive types in Mercury.
>> + % If you add a new alternative then you may also need to update the
>> + % following predicates:
>> + %
>> + % - parse_type_name.is_known_type_name_args/3
>> + % - inst_check.check_inst_defn_has_matching_type/7
>> + % - llds_out_data.output_type_ctor_addr/5
>> + % - type_util.classify_type_ctor/2
>> + %
>> :- type builtin_type
>> ---> builtin_type_int
>> ; builtin_type_uint
>> @@ -489,6 +499,10 @@ cons_id_is_const_struct(ConsId, ConstNum) :-
>> ; builtin_type_string
>> ; builtin_type_char.
>> +:- pred is_builtin_type_sym_name(sym_name::in) is semidet.
>> +:- pred is_builtin_type_name(string::in) is semidet.
> I am surprised that there is no place in the compiler that would want
> the builtin_ctor that corresponds to the name.
> Coming back to this: I see that type_ctor_info.m has a version of this
> code that does return the builtin_ctor. Is there any reason why you
> haven’t moved that functionality here? It would reduce code duplication.
It seems that type_ctor_info.m is the *only* place that wants it. In
any case, the table in type_ctor_info.m relates type names to values of
builtin_ctor0 type, not builtin_type/0. Moving into prog_data.m seems
like the wrong thing since the rest of the compliler doesn't need to
know about it.
>> --- a/compiler/special_pred.m
>> +++ b/compiler/special_pred.m
>> @@ -340,11 +340,7 @@ can_generate_special_pred_clauses_for_type(ModuleInfo, TypeCtor, TypeBody) :-
>> is_builtin_type_special_preds_defined_in_mercury(TypeCtor, TypeName) :-
>> Builtin = mercury_public_builtin_module,
>> TypeCtor = type_ctor(qualified(Builtin, TypeName), 0),
>> - ( TypeName = "int"
>> - ; TypeName = "uint"
>> - ; TypeName = "string"
>> - ; TypeName = "character"
>> - ; TypeName = "float"
>> + ( is_builtin_type_name(TypeName)
>> ; TypeName = "pred"
> This code expects the symname to have builtin.m as the qualifier module.
> The other code above all expected an *unqualified* name.
> Either this code is wrong (and has been for a while), or the tests on
> symnames in prog_data.m should accept both forms.
It's not a new problem, see https://bugs.mercurylang.org/view.php?id=13.
I'm not sure what the intended behaviour is meant to be (if indeed there
was ever such a thing):
1. all builtin types have unqualified names
2. all buitlin types have qualified names (e.g. with builtin)
3. there's a point in the compiler where the names switch from being
unqualified to qualified.
If it's either (1) or (2) then there are several points in the compiler
that are broken; if it's (3) then it's not sufficiently well documented.
In the interests of not inadvertently breaking stuff (further), I will
leave the above code as-is for now. (Note that type_ctor_info.m also
assumes that the builtin types are qualified.)
More information about the reviews