Re: Issue 5705: int->long in Stem::get_beaming and set_beaming (issue 561350044 by address@hidden)

[David Kastrup]

Han-Wen Nienhuys <address@hidden> writes:

> On Mon, Feb 3, 2020 at 2:40 PM Dan Eble <address@hidden> wrote:
>>
>> Han-Wen wrote:
>> > We could globally replace [int] with int64_t which has the
>> > additional advantage of being explicit about its size.
>>
>> I'm used to using those typedefs and I think they're beneficial, but
>> I don't expect that switching to them would be merely automatic,
>> given libraries that use basic C types (e.g. scm_ilength returning
>> long).  I'd expect some human judgment to be required.
>>
>> Han-Wen also wrote:
>> > Using a long here is 7 bytes too many.
>>
>> Do you believe that in general, no value should ever be stored in a
>> larger space than is necessary, or are you speaking specifically
>> about this beaming code being a memory hog?
>>
>> In my experience, for parameters, local variables, and return
>> values, it's usually better to reduce the number of conversions and
>> not worry about the size until there is evidence of a problem.  For
>> object data that will be used in a large number of instances,
>> concern is appropriate.
>
> My concern is this: the beaming number of a stem is a small integer
> (it is generally equal to Stem::duration_log - 2).  By making it a
> 'long',  the Stem class is now inconsistent with itself, because
> Stem::duration_log is an int, instead of a long.
>
> I am worried that if we continue making changes to types based on the
> signatures of libraries external to lilypond, the code will become a
> patchwork of types that are consistent with the external libraries
> (Guile, STL, etc.), but internally inconsistent with what things mean
> in the LilyPond codebase, ie. some small integers are 'int', some
> other set is 'long'.
>
> There is also a cognitive overhead. If most of the code uses "int" for
> standard integers, then places that use "long" are out of the
> ordinary, and they imply that there is something special going on that
> requires an extra-wide type. This will make the code harder to
> understand if there actually is no special reason.
>
> There are multiple ways out of this:
>
> 1) cast to the appropriate type as close to the external interface as
> possible (ie. cast scm_ilength() to an int directly)
>
> 2) standardize on a type that is wide enough for our purposes, ie.
> int64_t or long, so all of the code uses the same type, so we don't
> imply distinctions that are not there.
>
> 3) remove the convenience method Stem::get_beaming, and have all the
> callers deal with scm_ilength directly.
>
> 4) introduce a ly_int_length() which would encapsulate the cast in 1).

5) do not recompute the size information as an implicit property of the
list (which is an O(n) call every time it is needed) but rather keep it
around separately.

The principal problem we have here is that something starts as an
integer and then is only implicitly available as the length of a list.
Moving to an STL list would turn the length call into O(1) but it would
still be an implicitly available measure and the wrong type.

> I prefer 1) because it is simplest change to the existing code base,
> but there is a good case to make that 2) will require less
> case-by-case deliberation to make the cast warnings go away overall.

Until size_t and int64_t diverge which is the kind of thing that got us
all the warnings on moving to 64bit in the first place.

After looking at more of the code, it would appear that the way the list
is initialized, it cannot really have non-int length (as the
initialization count is an int).  If the length is supposed to become
large, scm_ilength is a bad idea to call anyway because of its cost.

> Speaking of rationals, it would be interesting to move to SCM values
> for Rationals instead of the current C++ class, but we'd need to be
> guile 2.x to garbage collection for values on the stack.

Guile 1.8 collects values on the stack.  That is not the problem.  The
problem is the values elsewhere, and we use, for example, Moment
elements in the Midi structs which are not SCM values.  The Midi data
structures are not scmified at all which would be a good idea for making
the Midi layer a whole lot more user-accessible than it is now.  But we
are not there yet, and as long as a Moment is in there, at least with
Guile 1.8 this part of the heap is not marked.

-- 
David Kastrup