[m-rev.] for review: optimize the tests for builtin types when printing terms

[Julien Fischer]

Hi Zoltan,

On Thu, 17 Oct 2019, Zoltan Somogyi wrote:

> On Thu, 17 Oct 2019 11:57:34 +1100 (AEDT), Julien Fischer <jfischer at opturion.com> wrote:

I'll address the bits about the type_ctor_rep enum later since they're
not relevant to the remainder of this review.
`
>> (print only special cases those types that it
>> represents differently from write.)
>
> That explains why it special cases a different set of types.
> But for the types that we special case in *both* write and print,
> it would be nice if the print code had a comment explaining
> *what the difference was*. I can see e.g. that we could print extra digits
> for floats in write for round-trippability, or add i8/u8/... suffixes after
> fixed size integral types, but don't see what we can do differently
> for int and uint itself.

uint gets a 'u' suffix in the write case, not in the print case.

> Are they special cased for speed?

For int I would imagine that's the case.  (It's also possible that
someone was being wildly optimistic about the compiler's ability to
inline and simplify code.)

The default case ends up going through deconstruct/5, for write that
does the wrong thing for the integer types (other than int) since it
omits the suffix.
(deconstruct/5 *does* do the right thing for integer types for print, but
print doesn't call it directly, it goes via write, which is probably
why print itself needs to handle them as a special case.)

> Only if they are, should float be special cased as well, for the same
> reason.

With your change, yes.

>>> +                    TB = type_builtin_string,
>>> +                    ( if univ_to_type(Univ, String) then
>>> +                        term_io.quote_string(Stream, String, !State)
>>> +                    else
>>> +                        write_ordinary_term(Stream, NonCanon, Univ, Priority,
>>> +                            !State)
>>> +                    )
>>
>> Why is this branch not just
>>
>>      TB = type_builtin_string,
>>      det_univ_to_type(Univ, String),
>>      term_io.quote_string(Stream, String, !State)
>>
>> ?  Similarly, for the other branches.
>
> det_univ_to_type internally does an if-then-else with the same condition,
> the only difference is it aborts in the else case. This would not be the right
> thing to do in two very unlikely but possible cases: that we add new type
> constructors to these modules with the same names but different arities,
> and that the user links into their programs their own modules with the
> same names but different contents as the standard library modules.
> The latter can't happen with builtin.m and private_builtin.m, since
> those are always implicitly imported anyway, but can happen with the others.

I suggest you put that explanation in a comment in the code.  I'm happy
for you to commit this after the above comments are addressed.

Julien.