Code review - Issue 557190043: lily: fix some type conversion warningshttps://codereview.appspot.com/2020-02-13T11:40:13+00:00rietveld
Message from unknown
2020-01-19T22:24:08+00:00hanwennurn:md5:5edca9482b0017853e397b282ef783d3
Message from lemzwerg@googlemail.com
2020-01-20T16:32:31+00:00lemzwergurn:md5:fde0013cffc53f1465bc780e26e6bbf2
From inspection only: LGTM, thanks!
https://codereview.appspot.com/557190043/diff/581490044/lily/lookup.cc
File lily/lookup.cc (right):
https://codereview.appspot.com/557190043/diff/581490044/lily/lookup.cc#newcode314
lily/lookup.cc:314: int i0 = (int) i;
Maybe 'int(i)' instead?
Message from nine.fierce.ballads@gmail.com
2020-01-21T15:21:56+00:00Dan Ebleurn:md5:d1a23da9a75fe4850eeeafc9f0ee20b4
https://codereview.appspot.com/557190043/diff/581490044/lily/pointer-group-interface.cc
File lily/pointer-group-interface.cc (right):
https://codereview.appspot.com/557190043/diff/581490044/lily/pointer-group-interface.cc#newcode30
lily/pointer-group-interface.cc:30: return arr ? int (arr->size ()) : 0;
MHO: Return a vsize. Somewhere, sometime, someone will probably say,
my_things_.size () < pgi.count ()
and the compiler will warn about comparing int to size_t.
I don't suggest that you switch to vsize and ferret out all the consequences at this time if you don't want to invest your time in that, but that it's better to leave this line alone and live with the warning until there is time to take a proper look.
https://codereview.appspot.com/557190043/diff/581490044/lily/quote-iterator.cc
File lily/quote-iterator.cc (right):
https://codereview.appspot.com/557190043/diff/581490044/lily/quote-iterator.cc#newcode104
lily/quote-iterator.cc:104: int hi = int (scm_c_vector_length (vec));
If scm_c_vector_length () is returning size_t, then why don't we work in terms of size_t? Any place I see a cast, especially a C-style cast, I feel the urge to stop and ask, "Why is this OK?" So I'd rather not see them where they can be avoided.
https://codereview.appspot.com/557190043/diff/581490044/lily/stem.cc
File lily/stem.cc (right):
https://codereview.appspot.com/557190043/diff/581490044/lily/stem.cc#newcode94
lily/stem.cc:94: int len = int (scm_ilength (lst)); // -1 for dotted lists!
ditto
Message from hanwenn@gmail.com
2020-01-22T10:15:03+00:00hanwennurn:md5:e6f5d61eebbe7430f022f4fa31284a5e
On 2020/01/21 15:21:56, Dan Eble wrote:
> https://codereview.appspot.com/557190043/diff/581490044/lily/pointer-group-interface.cc
> File lily/pointer-group-interface.cc (right):
>
> https://codereview.appspot.com/557190043/diff/581490044/lily/pointer-group-interface.cc#newcode30
> lily/pointer-group-interface.cc:30: return arr ? int (arr->size ()) : 0;
> MHO: Return a vsize. Somewhere, sometime, someone will probably say,
>
> my_things_.size () < pgi.count ()
>
> and the compiler will warn about comparing int to size_t.
>
> I don't suggest that you switch to vsize and ferret out all the consequences at
> this time if you don't want to invest your time in that, but that it's better to
> leave this line alone and live with the warning until there is time to take a
> proper look.
>
> https://codereview.appspot.com/557190043/diff/581490044/lily/quote-iterator.cc
> File lily/quote-iterator.cc (right):
>
> https://codereview.appspot.com/557190043/diff/581490044/lily/quote-iterator.cc#newcode104
> lily/quote-iterator.cc:104: int hi = int (scm_c_vector_length (vec));
> If scm_c_vector_length () is returning size_t, then why don't we work in terms
> of size_t? Any place I see a cast, especially a C-style cast, I feel the urge
> to stop and ask, "Why is this OK?" So I'd rather not see them where they can be
> avoided.
>
> https://codereview.appspot.com/557190043/diff/581490044/lily/stem.cc
> File lily/stem.cc (right):
>
> https://codereview.appspot.com/557190043/diff/581490044/lily/stem.cc#newcode94
> lily/stem.cc:94: int len = int (scm_ilength (lst)); // -1 for dotted lists!
> ditto
Can we go back to basics?
Why do we need to have the distinction between size_t and int? I know the standard library returns size_t in some places, but is there any reason for LilyPond to used unsigned integers anywhere?
I think the most practical solution is to cast any unsigned to int directly where we get it out of the external library.
Message from hanwenn@gmail.com
2020-01-22T10:18:48+00:00hanwennurn:md5:8930c1ec9ff9bd6927e7776801bfdfbc
On 2020/01/22 10:15:03, hanwenn wrote:
> On 2020/01/21 15:21:56, Dan Eble wrote:
> >
> https://codereview.appspot.com/557190043/diff/581490044/lily/pointer-group-interface.cc
> > File lily/pointer-group-interface.cc (right):
> >
> >
> https://codereview.appspot.com/557190043/diff/581490044/lily/pointer-group-interface.cc#newcode30
> > lily/pointer-group-interface.cc:30: return arr ? int (arr->size ()) : 0;
> > MHO: Return a vsize. Somewhere, sometime, someone will probably say,
> >
> > my_things_.size () < pgi.count ()
> >
> > and the compiler will warn about comparing int to size_t.
> >
> > I don't suggest that you switch to vsize and ferret out all the consequences
> at
> > this time if you don't want to invest your time in that, but that it's better
> to
> > leave this line alone and live with the warning until there is time to take a
> > proper look.
> >
> > https://codereview.appspot.com/557190043/diff/581490044/lily/quote-iterator.cc
> > File lily/quote-iterator.cc (right):
> >
> >
> https://codereview.appspot.com/557190043/diff/581490044/lily/quote-iterator.cc#newcode104
> > lily/quote-iterator.cc:104: int hi = int (scm_c_vector_length (vec));
> > If scm_c_vector_length () is returning size_t, then why don't we work in terms
> > of size_t? Any place I see a cast, especially a C-style cast, I feel the urge
> > to stop and ask, "Why is this OK?" So I'd rather not see them where they can
> be
> > avoided.
> >
> > https://codereview.appspot.com/557190043/diff/581490044/lily/stem.cc
> > File lily/stem.cc (right):
> >
> > https://codereview.appspot.com/557190043/diff/581490044/lily/stem.cc#newcode94
> > lily/stem.cc:94: int len = int (scm_ilength (lst)); // -1 for dotted lists!
> > ditto
>
> Can we go back to basics?
>
> Why do we need to have the distinction between size_t and int? I know the
> standard library returns size_t in some places, but is there any reason for
> LilyPond to used unsigned integers anywhere?
>
> I think the most practical solution is to cast any unsigned to int directly
> where we get it out of the external library.
alternately, we could just do -Wno-conversion in the makefile, and we wouldn't have to do any work here at all.
Message from benko.pal@gmail.com
2020-01-22T11:18:17+00:00benko.palurn:md5:bf471c06585b4e297fa419941a02a979
On 2020/01/22 10:15:03, hanwenn wrote:
> Why do we need to have the distinction between size_t and int? I know the
> standard library returns size_t in some places, but is there any reason for
> LilyPond to used unsigned integers anywhere?
It's not just the sign (btw I like unsigned integers):
they also differ in size, at least on 64 bit architectures
int is int32_t, size_t is uint64_t.
p
Message from nine.fierce.ballads@gmail.com
2020-01-23T20:02:58+00:00Dan Ebleurn:md5:a43dccfa79ac66309a4271fbf360d65b
On 2020/01/22 10:18:48, hanwenn wrote:
> alternately, we could just do -Wno-conversion in the makefile, and we wouldn't
> have to do any work here at all.
These warnings weren't implemented just to irritate us. They were implemented because C is the basement of C++ and there are bugs hiding in its dark corners.
My answer would be different if this were one of those categories of warnings with a significant number of false positives, but it isn't, as far as I've seen.
Message from hanwenn@gmail.com
2020-01-23T20:49:01+00:00hanwennurn:md5:a8fb14313359889f21e702e01c061eb8
On 2020/01/23 20:02:58, Dan Eble wrote:
> On 2020/01/22 10:18:48, hanwenn wrote:
> > alternately, we could just do -Wno-conversion in the makefile, and we wouldn't
> > have to do any work here at all.
>
> These warnings weren't implemented just to irritate us. They were implemented
> because C is the basement of C++ and there are bugs hiding in its dark corners.
That is a beautiful metaphor, but can you be a little more concrete? The integer conversion
is the same between C and C++, and the warnings are applied to all C code. That doesn't mean
that our C code would has the same kind of problems, and that the impact of those problems
warrant investment to address them.
I mean, if we were writing a new SSL library, I surely would want scrutiny on every cast, but
we aren't.
In LilyPond, the bulk of the warnings come from unsigned 64 bit sizes, ie. vector and list sizes.
In order to cause an error, you would have to create vector or list of over 2G of
elements (on 32 bit architecture), so the unsigned size would be converted to a negative integer. Since our objects
are all larger than a single byte, there isn't enough memory for this to be possible. (The same holds for 64 bit,
except that you can't buy machines with that much RAM)
Similarly, for causing a problem by converting 64-bit size_t to a 32-bit int, you'd have to create something
with a size of over 2e9. What kind of score do you have in mind that would cause this problem?
> My answer would be different if this were one of those categories of warnings
> with a significant number of false positives, but it isn't, as far as I've seen.
Can you point me to a conversion warning in LilyPond which could cause a genuine, user-visible problem?
Message from dak@gnu.org
2020-01-23T21:11:12+00:00dakurn:md5:770ecd4e7915cdba321198e0190ca1f3
hanwenn@gmail.com writes:
> In LilyPond, the bulk of the warnings come from unsigned 64 bit sizes,
> ie. vector and list sizes.
>
> In order to cause an error, you would have to create vector or list of
> over 2G of
> elements (on 32 bit architecture), so the unsigned size would be
> converted to a negative integer. Since our objects
> are all larger than a single byte, there isn't enough memory for this to
> be possible. (The same holds for 64 bit,
> except that you can't buy machines with that much RAM)
Reality check: my laptop is something like 10 years old and holds 16GB
of RAM.
--
David Kastrup
Message from nine.fierce.ballads@gmail.com
2020-01-23T21:43:32+00:00Dan Ebleurn:md5:f56183c4acaa3c1aa9be22595afc2d36
On 2020/01/23 20:49:01, hanwenn wrote:
> I mean, if we were writing a new SSL library, I surely would want scrutiny on
> every cast, but we aren't.
The question is, do you want scrutiny of any cast? If you do, then keeping the warnings enabled and keeping the signal-to-noise ratio high by casting only where it is well justified is the right approach. If you don't care, disable the warnings.
> Can you point me to a conversion warning in LilyPond which could cause a
> genuine, user-visible problem?
I'm not going to spend the time to sift through past commits, current warnings, and existing casts to answer this properly. One involving a shorter integer that sticks in my mind (because I was recently working on resolving it) involves MIDI track number, a 16-bit number. It would not surprise me to find places in the MIDI code where conversions to 8-bit numbers occur; perhaps there's a questionable one in there somewhere.
Now, I would prefer to leave off defending my stance on this, and let the senior contributors decide what is going to happen in this project.
Message from hanwenn@gmail.com
2020-01-24T08:49:29+00:00hanwennurn:md5:4c0da58c4e8f900a596602779c84ae13
On Thu, Jan 23, 2020 at 10:11 PM David Kastrup <dak@gnu.org> wrote:
> hanwenn@gmail.com writes:
>
> > In LilyPond, the bulk of the warnings come from unsigned 64 bit sizes,
> > ie. vector and list sizes.
> >
> > In order to cause an error, you would have to create vector or list of
> > over 2G of
> > elements (on 32 bit architecture), so the unsigned size would be
> > converted to a negative integer. Since our objects
> > are all larger than a single byte, there isn't enough memory for this to
> > be possible. (The same holds for 64 bit,
> > except that you can't buy machines with that much RAM)
>
> Reality check: my laptop is something like 10 years old and holds 16GB
> of RAM.
>
I meant: you can't buy a machine that has 2^63 bytes of RAM.
The hole in my argument here is that int is 32-bit even on 64-bit
architectures. I have been submersed in the world of the Go programming
language, where things are generally more sane, and "int" mirrors the size
of pointers.
Dan's argument (I think Dan is nine.fierce.ballads?) about the MIDI code is
a good argument to not switch off the -Werror=conversion error. Thanks for
setting me straight on that, Dan.
That said, for typesetting, a 32-bit signed size leaves room for 2e9
entities, and I claim that it is enough for anybody (an A4 page of music
typically 1k objects), so we can freely cast down to 32 bits when we get a
64 bit size_t somewhere. The alternative is that we don't cast it, but pass
on the 64 bit size_t, but every time we do that, we'll create new call
sites where we have to fix up conversions, so that will create more work
for ourselves, while we have many other more pressing issues to attend,
like resolving the GUILE 3 situation.
Thoughts?
--
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen
Message from dan@faithful.be
2020-01-24T11:29:30+00:00dan_faithful.beurn:md5:2e62c2d8c48e7d5b54ec4529cb20f66e
On Jan 24, 2020, at 03:49, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
>
> (I think Dan is nine.fierce.ballads?)
sort(Dan.full_name) == sort('ninE Fierce ballaDs')
—
Dan
Message from dan@faithful.be
2020-01-24T12:13:42+00:00dan_faithful.beurn:md5:5eb65472f9e113d83745be5aaca996f9
On Jan 24, 2020, at 03:49, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> The alternative is that we don't cast it, but pass on the 64 bit size_t, but every time we do that, we'll create new call sites where we have to fix up conversions, so that will create more work for ourselves,
I've been trickling in changes of this nature for a few months, and I'm content to keep it up if we can agree that it's the right thing to do.
—
Dan
Message from dak@gnu.org
2020-01-24T13:08:29+00:00dakurn:md5:c95bee0aa4edd41a8160a0a3a8189150
Dan Eble <dan@faithful.be> writes:
> On Jan 24, 2020, at 03:49, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
>> The alternative is that we don't cast it, but pass on the 64 bit
>> size_t, but every time we do that, we'll create new call sites where
>> we have to fix up conversions, so that will create more work for
>> ourselves,
>
> I've been trickling in changes of this nature for a few months, and
> I'm content to keep it up if we can agree that it's the right thing to
> do.
Personally I think it's not as much "the right" as a good thing to do.
We have stuff like the "Rational" type exploding for certain
denominators that the "new complexity" composers love to be dealing
with, and removing size mismatches at least lets them explode at a time
when their actual capacity rather than a fraction of it gets exhausted.
--
David Kastrup
Message from hanwenn@gmail.com
2020-01-30T08:37:44+00:00hanwennurn:md5:1e30d2597045961d2eb245de95459dff
There are two problems that are tangentially related, and I think the discussion comes from us wanting to solve different problems.
My problem is that it is very distracting to change the C++ code base if there are compiler warnings about existing code. So, I want a code base that compiles with zero warnings as a starting point.
There is a different problem, which that there are some dubious casts in our source code. I think that you, ie. Dan and David, want to address that problem.
Here is a proposal that can make both of us happy:
1) fix all warning provisorially, by adding the casts that the compiler does today explicitly. We mark them with "TODO: investigate cast". Result: the compile becomes warning-free
2) go over all the "investigate cast" warnings, changing return types/signatures where necessary. Result: our casts are now all kosher.
I'm happy to contribute the work for 1).
How does that sound?
Message from jonas.hahnfeld@gmail.com
2020-01-30T09:00:10+00:00hahnjourn:md5:bdc4b14d51efaa9a6a8a54b837f60777
On 2020/01/30 08:37:44, hanwenn wrote:
> There are two problems that are tangentially related, and I think the discussion
> comes from us wanting to solve different problems.
>
> My problem is that it is very distracting to change the C++ code base if there
> are compiler warnings about existing code. So, I want a code base that compiles
> with zero warnings as a starting point.
>
> There is a different problem, which that there are some dubious casts in our
> source code. I think that you, ie. Dan and David, want to address that problem.
>
> Here is a proposal that can make both of us happy:
>
> 1) fix all warning provisorially, by adding the casts that the compiler does
> today explicitly. We mark them with "TODO: investigate cast". Result: the
> compile becomes warning-free
>
> 2) go over all the "investigate cast" warnings, changing return types/signatures
> where necessary. Result: our casts are now all kosher.
>
> I'm happy to contribute the work for 1).
>
> How does that sound?
I don't like this methodology, what's the difference over disabling -Wconversion? (which I don't think would be a good idea)
I prefer what Dan does right now (btw thanks for working on this!): Pick a warning, investigate the issue and fix it correctly. Otherwise I see the risk that the TODOs will stick for longer than they should. So I think it's a good idea to keep the "distracting" warnings in place until they are correctly addressed.
Message from lemzwerg@googlemail.com
2020-01-30T09:20:52+00:00lemzwergurn:md5:4e9f347af9728d065d6cfa1c464b6746
> I don't like this methodology,
+1
> I prefer what Dan does right now (btw thanks for
> working on this!): Pick a warning, investigate
> the issue and fix it correctly.
I currently add `CXXFLAGS="-Wno-sign-conversion"` while calling make; this reduces the amount of warnings with clang to a level which looks acceptable to me.
Message from hanwenn@gmail.com
2020-02-02T14:15:04+00:00hanwennurn:md5:b3cc80052b8e21c9f47d480309ae5a70
> I don't like this methodology, what's the difference over disabling
> -Wconversion
My selfish is reason is that it gets the warnings out of the way of whomever is not interested in fixing casts, including myself.
The reason why this methodology is better approach overall is that it actually prevents from more dubious casts being added.
Right now, if I add another erroneous cast, it will generate a warning that looks similar to the zillions of warnings I already ignore, and it will need an eagle-eyed reviewer to catch the problem.
If the base compile is free of warnings, we can add -Werror to the compile, and any new errors will cause a failure in our CI, which is a much better way of preventing regressions.
Message from dan@faithful.be
2020-02-02T14:41:51+00:00dan_faithful.beurn:md5:98a3d207f6e029013f527bd40188248f
On Feb 2, 2020, at 09:15, hanwenn@gmail.com wrote:
>
>> I don't like this methodology, what's the difference over disabling
>> -Wconversion
>
> My selfish is reason is that it gets the warnings out of the way of
> whomever is not interested in fixing casts, including myself.
About 1/3 of the remaining warnings are for conversions between integer and floating point types. Those are most likely to be solved with static_cast<> in the long term, so I would have no objection if you approached _those_ that way.
(I encourage static_cast<> over C-style casts because static_cast is easier to grep for and the compiler is not so see-no-evil about it.)
> If the base compile is free of warnings, we can add -Werror to the
> compile, and any new errors will cause a failure in our CI, which is a
> much better way of preventing regressions.
This is an idea I can back. I recall that g++ supports something like -Werror -Wwarning=conversion (don't have time to check) that would allow treating most warnings as errors right now.
I do have a reservation when it comes to supporting many versions of compiler. I've been in the position of having to deal with false positive warnings from older compilers that were not well covered in CI. It was a drag.
-—
Dan
Message from hanwenn@gmail.com
2020-02-02T15:15:53+00:00hanwennurn:md5:bdc5b5d6299293daafa898fd77c816c4
On 2020/02/02 14:41:51, dan_faithful.be wrote:
> On Feb 2, 2020, at 09:15, mailto:hanwenn@gmail.com wrote:
> >
> >> I don't like this methodology, what's the difference over disabling
> >> -Wconversion
> >
> > My selfish is reason is that it gets the warnings out of the way of
> > whomever is not interested in fixing casts, including myself.
>
> About 1/3 of the remaining warnings are for conversions between integer and
> floating point types. Those are most likely to be solved with static_cast<> in
> the long term, so I would have no objection if you approached _those_ that way.
https://codereview.appspot.com/563460043/
> (I encourage static_cast<> over C-style casts because static_cast is easier to
> grep for and the compiler is not so see-no-evil about it.)
http://codereview.appspot.com/547560044
> > If the base compile is free of warnings, we can add -Werror to the
> > compile, and any new errors will cause a failure in our CI, which is a
> > much better way of preventing regressions.
>
> This is an idea I can back. I recall that g++ supports something like -Werror
> -Wwarning=conversion (don't have time to check) that would allow treating most
> warnings as errors right now.
right, but that will still leave warnings that are non-actionable for random
contributors in the compilation output, which is what I am arguing against
> I do have a reservation when it comes to supporting many versions of compiler.
> I've been in the position of having to deal with false positive warnings from
> older compilers that were not well covered in CI. It was a drag.
Hmm. Good point; because if we put -Werror inthe default compile flags, we'll create
problems for whomever is using the "wrong" version of the compiler.
Message from nine.fierce.ballads@gmail.com
2020-02-03T19:41:07+00:00Dan Ebleurn:md5:6022b9ddc27eeb29d8e320e5f323c06d
The changes to lily/break-substitution.cc are exactly what I would do. What do you think about splitting those into their own commit and pushing them?
Message from hanwenn@gmail.com
2020-02-03T19:55:33+00:00hanwennurn:md5:53d550507d4bd3fe3e701c46b6f56801
A-OK.
Message from unknown
2020-02-03T19:58:52+00:00hanwennurn:md5:89d28ff199f6bf5fa133a92a1cc44d63
Message from hanwenn@gmail.com
2020-02-03T19:58:54+00:00hanwennurn:md5:e9bc5cd8839a9418edd6b0aed67be20d
Dan's suggestion
Message from hanwenn@gmail.com
2020-02-13T11:40:13+00:00hanwennurn:md5:f4baf425b8540c93b7e192000a3074ad
commit 4d93339b06f97dcec176a47d9db0fc169acccef6
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date: Fri Jan 17 23:42:28 2020 +0100
lily: fix some type conversion warnings