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

[Dan Eble]

On Feb 3, 2020, at 06:27, David Kastrup <address@hidden> wrote:
> 
> address@hidden writes:
> 
>> sorry to complain late about this change. I understand that this gets
>> rid of a conversion warning, which is something that we want, but I am
>> missing the big picture here. Is there a plan for the big picture?
> 
> As far as I can see the big picture is matching the used variables to
> the used data types to that overflows become a non-issue.
...
> Just choosing the right types means that when a problem crops up likely
> due to an exceeded size, this is not a place you need to check up on and
> verify when going through all warning-silencing casts.

It's a fair question, especially on seeing a case where I chose to keep a 
signed type, after seeing many other cases where I changed a signed index to 
size_t.

"Choosing the right types" is a nice ideal, but there's plenty of pragmatism in 
this campaign, because I don't want to spend my whole life fixing warnings, but 
I very much want to avoid either hiding or introducing problems.  A type that 
seems more fitting on its face might require refitting a lot of code.  I do 
start out trying to understand the bigger picture, but at some point, the 
effort becomes more than I'm willing to spend.  This is one of those cases.  
Changing int to long seemed like a conservative way to eliminate the reason for 
the warning, and I tried to do it in a way that was coherent in the scope of 
the Stem code, if not in the big picture.

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.

I confess, I've not educated myself on how Guile stores int versus long, like 
whether it actually uses more space for scm_from_long(50) than for 
scm_from_int(50); however, this code didn't strike me as deserving that extra 
effort.  If you think it's important, I'm willing to revisit it and try to save 
space in the list while while still avoiding questionable silent conversions.
— 
Dan