[m-rev.] for review: Use consistent integer types for some RTTI fields.

Zoltan Somogyi zoltan.somogyi at runbox.com
Mon Nov 19 14:56:46 AEDT 2018



On Mon, 19 Nov 2018 14:37:00 +1100, Peter Wang <novalazy at gmail.com> wrote:
> > Changing the signedness of fields in the runtime is tricky, because
> > you will get warnings from the C compiler, which we normally regard
> > as errors.
> 
> One situation: we change a field from signed to unsigned in the runtime,
> but the bootstrap compiler populates the field with a *negative* value.

That is the potential problem.

> Do you have any other problems in mind?

Not *another problem*, but *another aspect* of the above problem.
You must make sure that the bootstrap compiler fills these fields
only with non-negative numbers not just on *your* machine, but on
*everyone else's machine. It is this waiting for everyone else to install
a new bootstrap compiler that I called "tricky".

> > This raises another point. What is the maximum number of function symbols
> > that a type is allowed to have? This code says that number must fit in 16 bits;
> > other code says only that it must fit in 32 bits. We should
> > 
> > - decide on one or the other,
> > - document the decision in the reference manual, and
> > - define a C type in the runtime and a Mercury type in the compiler
> >   that aliases uint16_t or uint32_t, and use that consistently.
> 
> By "this code", did you mean the int16 in the type of du_name_table?

Yes.

> That represents functor arities, not functor numbers.

I am almost certainly the one who wrote the old code here, and yet I did not
recognize that. This tells me even more strongly that introducing two new types,
named something like functor_arity (uint16) and functor_ordinal (uint32),
either as type synonyms or as notag types, would be a good idea.

> > We should do steps 2 and 3 above for the maximum arity
> > of a function symbol as well, though I don't think anyone will argue
> > for allowing arities that don't fit in 16 bits. For predicates, the maximum
> > arity we allow is either 1023 or 1024 (I don't recall which), and we should
> > probably use the same number here.
> 
> Seems fine.

Absent an objection from anyone else, will you update the reference manual,
or should I?

> > >  get_functor_du(TypeCtorRep, TypeInfo, TypeCtorInfo, FunctorNumber,
> > >          FunctorName, Arity, PseudoTypeInfoList, Names) :-
> > >      TypeFunctors = get_type_ctor_functors(TypeCtorInfo),
> > >      DuFunctorDesc = TypeFunctors ^ du_functor_desc(TypeCtorRep, FunctorNumber),
> > >  
> > >      FunctorName = DuFunctorDesc ^ du_functor_name,
> > > -    Arity = DuFunctorDesc ^ du_functor_arity,
> > > +    Arity = int16.to_int(DuFunctorDesc ^ du_functor_arity),
> > 
> > Any reason why you convert this to int here (and elsewhere in this module)?
> > 
> 
> In this case, Arity is passed to list.duplicate.
> In other cases, existing code or public interfaces use int for arities.
> I don't think there's any gain to sticking with int16s after the value
> has been extracted (and working with int16s increases the chances of
> overflow, admittedly by a miniscule amount in this case ;)

That is reasonable.

Zoltan.


More information about the reviews mailing list