Avoid some crashes for bad "control-points" property (issue 576610043 by dak@gnu.org)

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Avoid some crashes for bad "control-points" property (issue 576610043 by dak@gnu.org)

Thomas Morley-2
As always, I can't review C++.

So let me ask, how will LilyPond react seeing 5 or more control-points
after your patch?

https://codereview.appspot.com/576610043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Avoid some crashes for bad "control-points" property (issue 576610043 by dak@gnu.org)

David Kastrup
[hidden email] writes:

> As always, I can't review C++.
>
> So let me ask, how will LilyPond react seeing 5 or more control-points
> after your patch?
>
> https://codereview.appspot.com/576610043/

It just ignores additional control points and replaces missing ones with
(0 . 0).

--
David Kastrup

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Avoid some crashes for bad "control-points" property (issue 576610043 by dak@gnu.org)

benko.pal
In reply to this post by Thomas Morley-2
LGTM

https://codereview.appspot.com/576610043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Avoid some crashes for bad "control-points" property (issue 576610043 by dak@gnu.org)

Han-Wen Nienhuys-3
In reply to this post by Thomas Morley-2
add a regression test?

https://codereview.appspot.com/576610043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Avoid some crashes for bad "control-points" property (issue 576610043 by dak@gnu.org)

David Kastrup
[hidden email] writes:

> add a regression test?
>
> https://codereview.appspot.com/576610043/

I am not sure what the point of it would be.  This kind of bug may be
present elsewhere (and the regression test would not cover that) and it
is not likely to get reintroduced here since this code does not really
interact with other code.  And if you try running the test on older
versions for comparison, it will just crash.  The visual output is
meaningless since the condition it protects against does not have
meaning.

--
David Kastrup

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Avoid some crashes for bad "control-points" property (issue 576610043 by dak@gnu.org)

Han-Wen Nienhuys-3
In reply to this post by Thomas Morley-2
On 2019/04/26 18:49:03, dak wrote:
> mailto:[hidden email] writes:

> > add a regression test?
> >
> > https://codereview.appspot.com/576610043/

> I am not sure what the point of it would be.  This kind of bug may be
> present elsewhere (and the regression test would not cover that) and
it
> is not likely to get reintroduced here since this code does not really
> interact with other code.  And if you try running the test on older
> versions for comparison, it will just crash.  The visual output is
> meaningless since the condition it protects against does not have
> meaning.

After many years of developing software, I'm skeptic of any bugfix
without test, but yes, maybe it's overkill in this case? In any case, I
think there is a case in slur-configuration.cc too




https://codereview.appspot.com/576610043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Avoid some crashes for bad "control-points" property (issue 576610043 by dak@gnu.org)

David Kastrup
[hidden email] writes:

> On 2019/04/26 18:49:03, dak wrote:
>> mailto:[hidden email] writes:
>
>> > add a regression test?
>> >
>> > https://codereview.appspot.com/576610043/
>
>> I am not sure what the point of it would be.  This kind of bug may be
>> present elsewhere (and the regression test would not cover that) and
> it
>> is not likely to get reintroduced here since this code does not really
>> interact with other code.  And if you try running the test on older
>> versions for comparison, it will just crash.  The visual output is
>> meaningless since the condition it protects against does not have
>> meaning.
>
> After many years of developing software, I'm skeptic of any bugfix
> without test, but yes, maybe it's overkill in this case? In any case, I
> think there is a case in slur-configuration.cc too

You are right, and I'll have to fix that.  Seems my grep-fu is
lacklustre.  But a regression test would not have found it.  This kind
of bug seems more amenable to review.  Regression tests would protect
against badly resolved merge conflicts reintroducing old code, of
course, but our workflow does not really make them probable: there are
just too few people working on too few things.

I mean, feel free to propose a regtest.

> https://codereview.appspot.com/576610043/

--
David Kastrup

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Avoid some crashes for bad "control-points" property (issue 576610043 by dak@gnu.org)

Thomas Morley-2
In reply to this post by Thomas Morley-2
On 2019/04/28 19:14:53, dak wrote:
> Fix slur configuration too (thanks, Han-Wen)

I've downloaded you're patch. After playing around with it: all seems to
work nicely, no glitch found.
Thus, from testing:
LGTM

https://codereview.appspot.com/576610043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel