Ok, so it causes a difference even in code that did not fail. I cannot really see just what kind of difference and whether it would be relevant, namely either desirable or undesirable, or of little practical relevance.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Anonymous
-
2019-03-26
Description has changed:
Diff:
Needs: -->
Patch: new --> review
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Anonymous
-
2019-03-27
Patch: review --> countdown
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Anonymous
-
2019-03-27
Patch on countdown for March 30th
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi David,
could the regtest diff only be due to a different rounded value?
Also, I noticed that the same code that you fixed is also found in Stem_tremolo::calc_y_offset (but I haven’t found a way to trigger the crash with that), are we sure that it wouldn’t need to be rewritten into add_point as well?
The same failed assertion (or something very similar) is found in [#3374] and possibly in [#5281].
could the regtest diff only be due to a different rounded value?
That's unlikely. The change is invasive in that it not only triggers in cases that threw an assertion failure previously: it does not replace a lower bound but adds a point to an interval: that's only equivalent to the previous behavior if the replacement would not have shrunk the interval. I proposed it based on the handwaving guess that it would kick only in in sort-of pathological cases.
The problem now is that the regtest triggering shows that this change is not equivalent even in cases that are basic enough to be included in the regtests but did not previously trigger the problem: namely they shrink the interval but not to a degree of making it empty.
Since I was proposing this fix basically flying blindly, I don't know whether the change in behavior for cases that worked previously is likely a change to the better or worse. Extending the nature of the fix to other locations would similarly warrant making this decision.
So someone who has a clue what this code is actually likely to be doing (Keith?) should pitch in before we extend it to other areas. Likely even before we even commit in the current state.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Passes make, make check and a full make doc.
Reg test diff attached
Ok, so it causes a difference even in code that did not fail. I cannot really see just what kind of difference and whether it would be relevant, namely either desirable or undesirable, or of little practical relevance.
Diff:
Patch on countdown for March 30th
Hi David,
could the regtest diff only be due to a different rounded value?
Also, I noticed that the same code that you fixed is also found in Stem_tremolo::calc_y_offset (but I haven’t found a way to trigger the crash with that), are we sure that it wouldn’t need to be rewritten into add_point as well?
The same failed assertion (or something very similar) is found in [#3374] and possibly in [#5281].
Related
Issues: #3374
Issues: #5281
The problem now is that the regtest triggering shows that this change is not equivalent even in cases that are basic enough to be included in the regtests but did not previously trigger the problem: namely they shrink the interval but not to a degree of making it empty.
Since I was proposing this fix basically flying blindly, I don't know whether the change in behavior for cases that worked previously is likely a change to the better or worse. Extending the nature of the fix to other locations would similarly warrant making this decision.
So someone who has a clue what this code is actually likely to be doing (Keith?) should pitch in before we extend it to other areas. Likely even before we even commit in the current state.
I'll leave on Review - David but f you want to, you can put this on countdown (for April 2nd)
Patch on countdown for April 5th
Keith has relevant comments on the review that I need to address.
Passes make, make check and a full make doc.
Patch on countdown for April 15th.
Patch counted down, please push to staging.
Issue 5501: Avoid failed assertion in stem tremolo code master author David Kastrup <dak@gnu.org> Tue, 26 Mar 2019 00:02:52 +0100 (00:02 +0100) committer David Kastrup <dak@gnu.org> Mon, 15 Apr 2019 19:32:56 +0100 (20:32 +0200) commit 9d9dc557290e071a603c10a194ac4f5923bf0ce0