Issue 557190043: lily: fix some type conversion warnings (Closed)
Descriptionlily: fix some type conversion warnings
Patch Set 1 #
Total comments: 4
Patch Set 2 : Dan's suggestion #MessagesTotal messages: 23
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#newcod... lily/lookup.cc:314: int i0 = (int) i; Maybe 'int(i)' instead?
Sign in to reply to this message.
https://codereview.appspot.com/557190043/diff/581490044/lily/pointer-group-in... File lily/pointer-group-interface.cc (right): https://codereview.appspot.com/557190043/diff/581490044/lily/pointer-group-in... 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.c... 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
Sign in to reply to this message.
On 2020/01/21 15:21:56, Dan Eble wrote: > https://codereview.appspot.com/557190043/diff/581490044/lily/pointer-group-in... > File lily/pointer-group-interface.cc (right): > > https://codereview.appspot.com/557190043/diff/581490044/lily/pointer-group-in... > 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.c... > 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.
Sign in to reply to this message.
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-in... > > File lily/pointer-group-interface.cc (right): > > > > > https://codereview.appspot.com/557190043/diff/581490044/lily/pointer-group-in... > > 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.c... > > 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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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?
Sign in to reply to this message.
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.
Sign in to reply to this message.
> 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.
Sign in to reply to this message.
> 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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
A-OK.
Sign in to reply to this message.
Dan's suggestion
Sign in to reply to this message.
commit 4d93339b06f97dcec176a47d9db0fc169acccef6 Author: Han-Wen Nienhuys <hanwen@lilypond.org> Date: Fri Jan 17 23:42:28 2020 +0100 lily: fix some type conversion warnings
Sign in to reply to this message.
|
