[m-rev.] for review: mlds_to_{cs,java}_type.m

[Zoltan Somogyi]

2023-05-06 02:02 GMT+10:00 "Julien Fischer" <jfischer at opturion.com>:
>> diff --git a/compiler/mlds_to_cs_data.m b/compiler/mlds_to_cs_data.m
>> index c9138da65..620f326bc 100644
>> --- a/compiler/mlds_to_cs_data.m
>> +++ b/compiler/mlds_to_cs_data.m
>> @@ -317,14 +316,18 @@ output_cast_rval_for_csharp(Info, Type, Expr, Stream, !IO) :-
>>          output_rval_for_csharp(Info, Expr, Stream, !IO),
>>          io.write_string(Stream, ")", !IO)
>>      else if
>> +        % Given that the then-part and else-part of this if-then-else
>> +        % do exactly the same thing when the test succeeds, its only purpose
>> +        % can be optimization. However, I (zs) have strong doubts about
>> +        % whether this attempt at optimization actually works, because
>> +        % the gain, even when we get it, is very small.
> 
> It's not an optimization. The code was inherited from the Java backend (see
> below) where we need to add a field access if the r-value is actually an enum.
> This is because the Java backend currently represents enumerations as objects,
> whereas the C# represents them using C# enumerations and these can be converted
> to ints via a cast.
> 
>>          csharp_builtin_type(Type, "int")
>>      then
>>          io.write_string(Stream, "(int) ", !IO),
>>          output_rval_for_csharp(Info, Expr, Stream, !IO)
>>      else
>> -        io.write_string(Stream, "(", !IO),
>> -        output_type_for_csharp(Info, Type, Stream, !IO),
>> -        io.write_string(Stream, ") ", !IO),
>> +        io.format(Stream, "(%s) ",
>> +            [s(type_to_string_for_csharp(Info, Type))], !IO),
>>          output_rval_for_csharp(Info, Expr, Stream, !IO)
>>      ).
> 
> ...
> 
>> diff --git a/compiler/mlds_to_java_data.m b/compiler/mlds_to_java_data.m
>> index 935c364b6..00072817d 100644
>> --- a/compiler/mlds_to_java_data.m
>> +++ b/compiler/mlds_to_java_data.m
> 
> ...
> 
>> @@ -330,21 +329,25 @@ output_cast_rval_for_java(Info, Type, Expr, Stream, !IO) :-
>>          % XXX We really should be able to tell if we are casting a
>>          % TypeCtorInfo or a TypeInfo. Julien says that's probably going to
>>          % be rather difficult as the compiler doesn't keep track of where
>> -        % type_ctor_infos are acting as type_infos properly.
>> +        % type_ctor_infos are acting as type_infos properly. (zs agrees.)
>>          maybe_output_comment_for_java(Info, Stream, "cast", !IO),
>>          io.write_string(Stream,
>>              "jmercury.runtime.TypeInfo_Struct.maybe_new(", !IO),
>>          output_rval_for_java(Info, Expr, Stream, !IO),
>>          io.write_string(Stream, ")", !IO)
>>      else if
>> +        % Given that the then-part and else-part of this if-then-else
>> +        % do exactly the same thing when the test succeeds, its only purpose
>> +        % can be optimization. However, I (zs) have strong doubts about
>> +        % whether this attempt at optimization actually works, because
>> +        % the gain, even when we get it, is very small.
> 
> That comment is not correct; special handling is required when the r-value
> is an enum.

The attached diff addresses both these concerns, and contains other changes
as well.  Thank you.

Was that email the entire review, or is there more to come?
 
Zoltan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Log.m2x2
Type: application/octet-stream
Size: 782 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20230508/a2d7e0ca/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DIFF.m2x2
Type: application/octet-stream
Size: 27475 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20230508/a2d7e0ca/attachment-0003.obj>