Reviewers: Neil Puttock,
http://codereview.appspot.com/475041/diff/1/10
File lily/context-mod.cc (right):
http://codereview.appspot.com/475041/diff/1/10#newcode73
lily/context-mod.cc:73: Context_mod::to_alist () const
On 2010/03/13 23:24:24, Neil Puttock wrote:
I'd name this something like get_mods (), otherwise it implies you're
building
an alist to return (which obviously isn't the case)
Yes, of course. I simply copied Context_def (where we really return an
alist) and adjusted the code, but overlooked this little detail...
http://codereview.appspot.com/475041/diff/1/10#newcode75
lily/context-mod.cc:75: return mods_;
reverse is missing here (each new mod is prepended!). At first look it
might not make a difference, but it does if a block contains two
conflicting settings. Inside \context, the last one is applied, if we
don't reverse mods_ here, the first takes precedence!
In particular, if we have
\remove "Clef_engraver"
\consists "Clef_engraver"
inside a \context {\Staff ...} block, the staff will contain the clef
engraver, while inside a \with block, it would be removed...
http://codereview.appspot.com/475041/diff/1/11
File lily/include/context-mod.hh (right):
http://codereview.appspot.com/475041/diff/1/11#newcode30
lily/include/context-mod.hh:30: // #include "lily-proto.hh"
On 2010/03/13 23:24:24, Neil Puttock wrote:
Add prototype for Context_mod to lily-proto.hh ?
Hmm, you are right, that might be a good idea (just in case some class
later on will work with pointers to Context_mod...). Should I then
include lily-proto.hh, even if technically not required?
http://codereview.appspot.com/475041/diff/1/11#newcode49
lily/include/context-mod.hh:49: VIRTUAL_COPY_CONSTRUCTOR(Context_mod,
Context_mod);
On 2010/03/13 23:24:24, Neil Puttock wrote:
VIRTUAL_COPY_CONSTRUCTOR (
Hehe, that's copied verbatim from Context_def...
(BTW, a grep shows 157 lines with missing spaces just in lily/*.cc, and
203 lines with missing spaces in lily/include/*.hh -- Shall we fix them
all in one go?)
http://codereview.appspot.com/475041/diff/1/12
File lily/lily-lexer.cc (right):
http://codereview.appspot.com/475041/diff/1/12#newcode52
lily/lily-lexer.cc:52: {"contextModifications", CONTEXT_MOD},
On 2010/03/13 23:24:24, Neil Puttock wrote:
contextmodifications (all the parser keywords are lower case)
That keywords looks absolutely awkward. Maybe we should revisit that
convention... The alternative would be to not introduce a new parser
keyword at all and simply use \with instead (although that does not
describe what the following block is supposed to do when assigned to a
variable, i.e. "mymods = \with { \consists.... }"
http://codereview.appspot.com/475041/diff/1/13
File lily/parser.yy (right):
http://codereview.appspot.com/475041/diff/1/13#newcode1069
lily/parser.yy:1069: context_mod_list:
Unfortunately something here breaks \consists Some_engraver (without
quotes!). The parser now tries to evaluate Some as a variable rather
than interpret "Some_engraver" as a string...
http://codereview.appspot.com/475041/diff/1/14
File ly/engraver-init.ly (right):
http://codereview.appspot.com/475041/diff/1/14#newcode466
ly/engraver-init.ly:466: }
Shall we add a convert-ly rule for \RemoveEmptyStaffContext ->
\Staff\RemoveEmptyStaves ? Of course, a simplar rule for the other
RemoveEmpty*StaffContext's is needed, too.
Description:
Context mods stored in variable, can be inserted into \with or \context
-) Context-Modifications: create C++ class to store them
-) context modifications lists are stored in a dedicated simple scheme
object
(C++ class Context_mod)
-) Changes to the parser:
-) context_modifications objects (stored in variables) are now also
allowed
with \with clauses
-) context_modifications objects are also allowed inside \context
-) this allows us to rewrite \RemoveEmptyStaffContext (unfortunately
with
a little different syntax, since we no longer store \Staff inside the
\RESC command) so that it no longer erases previous settings to the
Staff context. Now, instead of
\context { \RemoveEmptyStaffContext }
one can do
\context { \Staff \RemoveEmptyStaves }
with the same effect and preserve previous changes to the Staff
context.
(The same applies of course to \DrumStaff, \RhythmicStaff, etc. as
well)
-) Adjusted engraver-init.ly and the regtests accordingly; Also added
regtest
that checks for RESC not discarding previous settings to the Staff
context
Please review this at http://codereview.appspot.com/475041/show
Affected files:
A input/regression/context-mod-context.ly
A input/regression/context-mod-with.ly
M input/regression/hara-kiri-drumstaff.ly
A input/regression/hara-kiri-keep-previous-settings.ly
M input/regression/hara-kiri-percent-repeat.ly
M input/regression/hara-kiri-pianostaff.ly
M input/regression/hara-kiri-rhythmicstaff.ly
M input/regression/hara-kiri-tabstaff.ly
A lily/context-mod.cc
A lily/include/context-mod.hh
M lily/lily-lexer.cc
M lily/parser.yy
M ly/engraver-init.ly
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
