Menu
▾
▴
#3199 Patch: Uses only unpure-pure containers to articulate unpure-pure relationships.
Originally created by: *anonymous
Originally created by: mts...@gmail.com
Originally owned by: mts...@gmail.com
Uses only unpure-pure containers to articulate unpure-pure relationships.
Previously, LilyPond used several different lists in define-grobs.scm
to define relationships between unpure and pure functions. This patch
eliminates these lists, using unpure-pure containers to articulate
these relationships.
The modifications required to implement this change are described below:
-) axis-group-interface.cc
A Scheme function is no longer needed to determine pure relevant grobs.
All grobs can have their Y-extents meaningfully pure-evaluated now. The
worst-case scenario is that they evaluate to false. Dead grobs, on the
other hand, are never pure relevant. The calls to is_live are the only
holdovers from the old pure-relevant? scheme function.
-) grob-closure.cc
We allow unpure-pure containers in simple closures.
-) grob-property.cc
call_pure_function no longer looks up a Scheme module. Because there are
no hard-coded lists in Scheme any more, the function is smaller and
writing it in C++ gets slight efficiency gains.
-) grob.cc
pure_stencil_height used to be a Scheme function in define-grobs.scm.
Because it is much simpler (it no longer makes references to lists defined
in Scheme), it can be implemented in C++.
-) pure-from-neighbor-engraver.cc
Similar to axis-group-interface.cc, the pure-relevant distinction is
no longer important.
-) side-position-interface.cc
In order to avoid issues with alterBroken, we only check pure properties
before line breaking.
-) simple-closure.cc
Simple closures were incorrectly evaluated when containing unpure-pure
containers. This rectifies that.
-) stencil-integral.cc
Several pure equivalent functions needed to be written for skylines.
-) define-grobs.scm
Multiple overrides must be changed to unpure-pure containers. Previous
hard-coded lists are all deleted and several functions moved to C++ (see
above).
-) output-lib.scm
Several common unpure-pure containers used in define-grobs.scm are
defined here. Several functions from define-grobs.scm pertaining to
grob offsets are moved to this file.
Originally posted by: dak@gnu.org
Oh crap.
SCM
pure_mark (SCM pure)
{
scm_gc_mark (unpure_pure_container_unpure_part (pure));
scm_gc_mark (unpure_pure_container_pure_part (pure));
return pure;
}
That is definitely not the right place to conjure a fresh closure when calling unpure_pure_container_pure_part. Returning pure is redundant, by the way. That one is marked anyway. I'll commit a fix. Guess I was lucky (or unlucky) that this did not explode in my hands when testing.
Originally posted by: dak@gnu.org
Pushed a fix to staging. Sorry for that.
Originally posted by: mts...@gmail.com
Uses single-argumented ly:make-unpure-pure-container
http://codereview.appspot.com/7377046
Labels: -Patch-review Patch-new
Originally posted by: dak@gnu.org
Patchy the autobot says: passes tests. Results quite the same as with previous version, including the increased memory usage and the single visual test difference.
Labels: -Patch-new Patch-review
Originally posted by: mts...@gmail.com
Pushed as [r74e4d219b24ec6d6f28d663c0285418e6c8e122e]
Labels: -Patch-review Patch-push Fixed_2_17_14
Status: Fixed
Originally posted by: dak@gnu.org
I have to admit that I am annoyed at having a complex patch like this pushed when it is not in state "Patch-countdown" or "Patch-push". It is hard enough trying to do thorough reviews without being forced to second-guess which patches are going to get pushed without warning.
Originally posted by: mts...@gmail.com
The patch was on countdown till last night, there was one comment from you regarding pure containers that I addressed, and it was never reset to patch needs_work. The countdown ended @ 8pm my time last night, so at 8:30ish I cleaned it up and pushed it just cuz time is short over the next few days. Sorry if that jumped the gun.
In general, with countdowns, I guess I don't completely understand how things work. I was operating under the assumption that if no one complained, things could be pushed after a countdown ended. There are two of my patches that were put back on the countdown although I'm not sure why, which is not a big deal except that it takes up a bit more of my time each week to re-rebase these things (I spent a half hour doing that last night on a patch that was put back on the countdown). In the CG, we say:
"On the scheduled countdown day, the Patch Handler reviews the previous list of patches on countdown, with the same procedure and criteria as before. Patches with no controversy can be set to “patch-push” with a courtesy message added to the comment block."
There was no controversy, I set to patch-push and pushed. Lemme know what I'm missing, as I want to make sure I get this right in the future.
Originally posted by: dak@gnu.org
"It was never reset" implies some magical fairy. We don't have magical fairies here. There are two possibilities to arrive at "Patch-countdown": one is the Patch meister setting it because sufficient time has passed in Patch-review state. The Patch Meister basically works via timing, not via making technical decisions. The other possibility is setting it back _manually_ to Patch-countdown right after Patchy has given its ok. This can be done by the Patch submitter if all of the following apply:
a) there is a pressing need for the patch
b) the change causing the countdown to be interrupted is not a game changer needing new review
c) the patch state has been Patch-countdown for the large majority of the countdown duration, so it has had a reasonable chance of being viewed as on countdown.
d) it has been originally set to Patch-countdown by the Patch Meister, and any changes to that state were of short duration and minor nature.
A Patch that is not marked Patch-countdown is _not_ on countdown. Never ever. Period. Otherwise our whole review system becomes a mockery.
The status "Patch-push" means "the patch has passed a regular countdown and can be pushed with a procedural ok by the Patch Meister". Nobody _ever_ puts a patch to "Patch-push" except the Patch Meister. After a patch has been pushed, Patch-push gets reset.
Sometimes there are reasons to bypass procedures on your own responsibility. Bypassing procedures means pushing without a procedural ok and taking responsibility for it. It does not mean lying about the procedures and assigning blame to the Patch Meister for things he did not do.
If you feel uncomfortable pushing a change that has not been given state "Patch-push", the solution is not giving it state "Patch-push" but rather not pushing it.
"Patch-push" is a blessing you don't get to bestow on yourself. If you have forgotten to put a patch back to Patch-countdown timely after a minor interruption of the countdown, the solution is not to ignore this and push anyway, but wait for the next countdown.
There is _no_ point whatsoever raving at the Patch Meister for not putting the state of a patch back to Patch-countdown after the patch say changes. The decision whether any changes were trivial enough to warrant resuming the countdown is a technical decision and out of the league of the Patch Meister.
If you don't feel comfortable making this technical decision, then you wait for the patch to be put on the next countdown.
All this is common sense if you bother thinking about it and not considering everybody else's life to revolve around your workflows and eager to spend every waking minute on second-guessing you and your work.
And it is not exactly like this is the first time it has been explained.
Originally posted by: dak@gnu.org
Another note. You wrote
> In the CG, we say:
> "On the scheduled countdown day, the Patch Handler reviews the previous list of patches on countdown, with the same procedure and criteria as before. Patches with no controversy can be set to “patch-push” with a courtesy message added to the comment block."
You are not the "Patch Handler". James is, and you are sabotaging his
job.
Originally posted by: Elu...@gmail.com
(No comment was entered for this change.)
Status: Verified
Originally posted by: k-ohara5...@oco.net
A revert, with conflicts resolved, in case we need it
https://codereview.appspot.com/9229045/
.