[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Issue 5705: int->long in Stem::get_beaming and set_beaming (issue 56
From: |
Dan Eble |
Subject: |
Re: Issue 5705: int->long in Stem::get_beaming and set_beaming (issue 561350044 by address@hidden) |
Date: |
Mon, 3 Feb 2020 08:40:11 -0500 |
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