Re: 2.15.8 Regtests
On Sat, Aug 06, 2011 at 11:48:30PM +, Keith OHara wrote: David Kastrup dak at gnu.org writes: Looks like we associate differing meanings with one or several of the words unless, somebody or objects. If the test remains in input/regression, then anyone inspecting the tests will think that \revert of nested properties worked completely in version 2.14 but now fails. Agreed; we should try to avoid giving that impression. Does anyone object to removing the test 'nested-property-revert.ly' from the input/regression? I believe that we should (temporarily) remove input/regression/nested-property-revert.ly from git master. Note that the file itself will still be in the git history, and we can unearth it using webgit (or your local git) whenever we want. So no need to panic about making a copy right now before it is removed. I believe that once we have cleared up the relevant fundamental architecture brokenness, we should reinstate that test (or at least, something like it). But I think that we should treat that reinstatement like a new patch, i.e. include it with whatever programming fix patch. I think this is exactly what Keith was proposing, but I hope that this explanation is more clear for the email archives. Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: 2.15.8 Regtests
Phil Holmes m...@philholmes.net writes:
One major change from 2.15.7 - in nested-property-revert.ly. I'm
assuming this is down to David's reversion of an earlier commit. I'm
not expert enough to say what should happen here, although it doesn't
look right to me.
It's work in progress, I am afraid. If you take a look at the source
code, you'll see that it is quite dubious:
\version 2.14.0
\header {
texidoc =
If a nested property revert follows an override in the same grob for
a different property, the nested property's default setting should not
be evicted from the property alist.
}
\relative c' {
c1\startTrillSpan
c1\stopTrillSpan
\override TrillSpanner #'color = #red
\revert TrillSpanner #'(bound-details left text)
c1\startTrillSpan
c1\stopTrillSpan
}
This should not is not achievable with the current data structures
without causing other regressions even harder to explain (which already
happened). So I picked the less complex code and behavior as a starting
point for first fixing the data structures, then the code.
The test case is quite artificial and, in my opinion, not worth fixing
temporarily at the cost of breaking more straightforward code.
So this is work in progress. I am on it, but currently I need to find
the source of a segmentation fault. If we are lucky, it is the same
already marked down as critical.
--
David Kastrup
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: 2.15.8 Regtests
- Original Message -
From: David Kastrup d...@gnu.org
To: lilypond-devel@gnu.org
Sent: Saturday, August 06, 2011 3:04 PM
Subject: Re: 2.15.8 Regtests
Phil Holmes m...@philholmes.net writes:
One major change from 2.15.7 - in nested-property-revert.ly. I'm
assuming this is down to David's reversion of an earlier commit. I'm
not expert enough to say what should happen here, although it doesn't
look right to me.
It's work in progress, I am afraid. If you take a look at the source
code, you'll see that it is quite dubious:
\version 2.14.0
\header {
texidoc =
If a nested property revert follows an override in the same grob for
a different property, the nested property's default setting should not
be evicted from the property alist.
}
\relative c' {
c1\startTrillSpan
c1\stopTrillSpan
\override TrillSpanner #'color = #red
\revert TrillSpanner #'(bound-details left text)
c1\startTrillSpan
c1\stopTrillSpan
}
This should not is not achievable with the current data structures
without causing other regressions even harder to explain (which already
happened). So I picked the less complex code and behavior as a starting
point for first fixing the data structures, then the code.
The test case is quite artificial and, in my opinion, not worth fixing
temporarily at the cost of breaking more straightforward code.
So this is work in progress. I am on it, but currently I need to find
the source of a segmentation fault. If we are lucky, it is the same
already marked down as critical.
I think this needs to be added to the tracker, and would normally count as
critical regression. I'm tempted to mark it regression - high - since it's
only there because of another critical that you fixed (IIRC) and you are
working on it. You OK with that?
--
Phil Holmes
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: 2.15.8 Regtests
Phil Holmes m...@philholmes.net writes: Phil Holmes m...@philholmes.net writes: One major change from 2.15.7 - in nested-property-revert.ly. I'm assuming this is down to David's reversion of an earlier commit. I'm not expert enough to say what should happen here, although it doesn't look right to me. This should not is not achievable with the current data structures without causing other regressions even harder to explain (which already happened). So I picked the less complex code and behavior as a starting point for first fixing the data structures, then the code. The test case is quite artificial and, in my opinion, not worth fixing temporarily at the cost of breaking more straightforward code. I think this needs to be added to the tracker, and would normally count as critical regression. I'm tempted to mark it regression - high - since it's only there because of another critical that you fixed (IIRC) and you are working on it. You OK with that? I have a hard time counting the removal of a band aid for an artificial test case with undefined behavior (try finding a place in the user documentation that declares this kind of code as producing predictable results) as a regression because the original code did not fix the underlying problem, but merely masked it. As a rather silly analogy: imagine that I put exit(0) right at the start of the main program. Somebody takes it out eventually, and I demand that every segfault needs now to be labelled as a regression since Lilypond did not segfault with my fix installed. Bluntly: unmatched nested overrides/reverts never ever worked sensibly, and actually their behavior never has been defined in the user documentation either. That we temporarily had a fix that was hand-crafted to match the only test-case in our regression tests in order to make just this test produce more intuitive results, does not change that. So I don't see the regression, and frankly, I don't see the high importance as the feature could not be used predictably at any point of time. It is a rather new and, obviously, largely untested feature. If you tread carefully (and this test case doesn't), you are likely to get what you expect. There is a cheap workaround: don't revert a nested property that you did not override in the same manner and order. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: 2.15.8 Regtests
On 6 August 2011 15:31, David Kastrup d...@gnu.org wrote:
I have a hard time counting the removal of a band aid for an artificial
test case with undefined behavior (try finding a place in the user
documentation that declares this kind of code as producing predictable
results) as a regression because the original code did not fix the
underlying problem, but merely masked it.
So how would you expect the following code to behave? It's the
snippet from the original bug report, which segfaulted in stem.cc.
\relative c' {
\time 2/4
\voiceOne
s16 [g s g ] s16 [g s g ] |
s16 [g s g ] \override Stem #'(details beamed-lengths) = #'(15 15)
s16 [g s g ] |
s16 [g s g ] s16 [g s g ] |
s16 [g s g ] \revert Stem #'(details beamed-lengths) s16 [g s g ] |
s16 [g s g ] s16 [g s g ] |
}
The regression test is deliberately artificial since it gives a clear
indication of failure, which this code doesn't (the segfault no longer
occurs due to checking the nested property is a pair before using
robust_list_ref). I don't think it's unreasonable to expect this code
to return 'beamed-lengths to the default value defined in
define-grobs.scm.
Cheers,
Neil
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: 2.15.8 Regtests
Neil Puttock n.putt...@gmail.com writes:
On 6 August 2011 15:31, David Kastrup d...@gnu.org wrote:
I have a hard time counting the removal of a band aid for an artificial
test case with undefined behavior (try finding a place in the user
documentation that declares this kind of code as producing predictable
results) as a regression because the original code did not fix the
underlying problem, but merely masked it.
So how would you expect the following code to behave? It's the
snippet from the original bug report, which segfaulted in stem.cc.
\relative c' {
\time 2/4
\voiceOne
s16 [g s g ] s16 [g s g ] |
s16 [g s g ] \override Stem #'(details beamed-lengths) = #'(15 15)
s16 [g s g ] |
s16 [g s g ] s16 [g s g ] |
s16 [g s g ] \revert Stem #'(details beamed-lengths) s16 [g s g ] |
s16 [g s g ] s16 [g s g ] |
}
The regression test is deliberately artificial since it gives a clear
indication of failure, which this code doesn't (the segfault no longer
occurs due to checking the nested property is a pair before using
robust_list_ref). I don't think it's unreasonable to expect this code
to return 'beamed-lengths to the default value defined in
define-grobs.scm.
The problem is that you can't reliably distinguish
\override Stem #'(details) = #'(beamed-lengths 15 15)
from
\override Stem #'(details beamed-lengths) = #'(15 15)
Maybe something like
(eq? (assq-ref (cdr alist) (caar alist)) (cddar alist))
could determine whether we are still on the spine leading to the
nested override or beyond it, but I am skeptical, and it still does not
solve the problem that updating a nested property in a parenting context
will require updating of the copies in the current context.
In any case, I am not interested in policies. Mark this whatever you
like, use whatever inconsistent behavior you like. The smaller code is
more useful as a starting point for reasonably sensible and correct
behavior, and I never suggested that it stay in either of the incorrect
states.
--
David Kastrup
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: 2.15.8 Regtests
Phil Holmes mail at philholmes.net writes: Phil Holmes mail at philholmes.net writes: One major change from 2.15.7 - in nested-property-revert.ly. I'm assuming this is down to David's reversion of an earlier commit. I think this needs to be added to the tracker, and would normally count as critical regression. Phil, No need for a new tracked issue. 'nested-property-revert.ly' was the test for issue 1063. It is now clear that issue 1063 was never really fixed. The goal was to make \revert restore the default value in more situations. The patch made 'nested-property-revert.ly' work, but made 'property-nested-revert.ly' fail, as noted in issue 1771. These similarly-named tests cover the same concept. Version 2.14 did not actually get the desired behavior that issue 1063 asked for, so there is no meaningful regression. Unless somebody objects, I will remove 'nested-property-revert.ly' from the test suite; The test code still exists in the open issue 1063. ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: 2.15.8 Regtests
Keith OHara k-ohara5...@oco.net writes: No need for a new tracked issue. 'nested-property-revert.ly' was the test for issue 1063. It is now clear that issue 1063 was never really fixed. The goal was to make \revert restore the default value in more situations. The patch made 'nested-property-revert.ly' work, but made 'property-nested-revert.ly' fail, as noted in issue 1771. These similarly-named tests cover the same concept. Version 2.14 did not actually get the desired behavior that issue 1063 asked for, so there is no meaningful regression. Unless somebody objects, I will remove 'nested-property-revert.ly' from the test suite; The test code still exists in the open issue 1063. I'd like to keep this test as I am working on the full functionality. It will likely get extended in the end to cover more cases, but it is useful as a start already. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: 2.15.8 Regtests
David Kastrup dak at gnu.org writes: Keith OHara k-ohara5a5a at oco.net writes: Unless somebody objects, I will remove 'nested-property-revert.ly' from the test suite; The test code still exists in the open issue 1063. I'd like to keep this test as I am working on the full functionality. It will likely get extended in the end to cover more cases, but it is useful as a start already. Thanks, David. Since you are keeping a copy, and there is a copy on 1063, I will remove it from input/regression right away. Don't forget to make an extra copy before you next rebase the branch on which you are working on 1063; you will probably need to `git add` it back to that branch. - Keith ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: 2.15.8 Regtests
Keith OHara k-ohara5...@oco.net writes: David Kastrup dak at gnu.org writes: Keith OHara k-ohara5a5a at oco.net writes: Unless somebody objects, I will remove 'nested-property-revert.ly' from the test suite; The test code still exists in the open issue 1063. I'd like to keep this test as I am working on the full functionality. It will likely get extended in the end to cover more cases, but it is useful as a start already. Thanks, David. Since you are keeping a copy, and there is a copy on 1063, I will remove it from input/regression right away. Don't forget to make an extra copy before you next rebase the branch on which you are working on 1063; you will probably need to `git add` it back to that branch. Looks like we associate differing meanings with one or several of the words unless, somebody or objects. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: 2.15.8 Regtests
David Kastrup dak at gnu.org writes: Looks like we associate differing meanings with one or several of the words unless, somebody or objects. Oops. I'm delaying pushing. The troublesome word was the verb keep, from I'd like to keep. Of course you can keep it, on your branch. If the test remains in input/regression, then anyone inspecting the tests will think that \revert of nested properties worked completely in version 2.14 but now fails. Does anyone object to removing the test 'nested-property-revert.ly' from the input/regression? ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
