Re: Allow (closed) scheme function calls as text scripts. (issue 6812088)
Hi David, On Thu, Nov 8, 2012 at 9:27 AM, d...@gnu.org wrote: [snip detailed explanation] thanks for explaining and sorry for not looking this up myself - i'm short on time and thus i do all reviews in a hurry... The commit message might be misleading, however, since it sounds like it is touting a fundamental new feature. What this is is more like putting another piece in place for the general idea of strings/markups can be submitted like any Scheme expression would. It is more like plugging an oversight rather than adding something new. Ok, i see your point. However, since original author of that code did it wrong, i still believe that it'd be good to make information about it somewhat more visible, so that other people could learn from that mistake. You may just dump this info into GC miscellaneous. Anyway, feel free to do whatever you decide, and LGTM :) Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow (closed) scheme function calls as text scripts. (issue 6812088)
On 2012/11/05 22:45:06, thomasmorley65 wrote: On 2012/11/05 22:40:36, lemzwerg wrote: Very nice! Can't review the code. But from description: GREAT The same in my case... David, could you add a comment (either to the parser code, or in the commit message) that explains why the old code didn't work and how the new code solves the problem? Even if it's obvious for you, I think it would be helpful for anyone reading and writing parser code in the future. best, Janek http://codereview.appspot.com/6812088/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow (closed) scheme function calls as text scripts. (issue 6812088)
On 2012/11/08 08:28:40, janek wrote: On 2012/11/05 22:45:06, thomasmorley65 wrote: On 2012/11/05 22:40:36, lemzwerg wrote: Very nice! Can't review the code. But from description: GREAT The same in my case... David, could you add a comment (either to the parser code, or in the commit message) that explains why the old code didn't work and how the new code solves the problem? Even if it's obvious for you, I think it would be helpful for anyone reading and writing parser code in the future. The problem with a code comment here is it would be more or less putting lipstick on a pig. The grammar consists of rules and alternatives, partly dictated by functionality, partly by constraints. The change here was a single-word change, substituting embedded_scm_bare with embedded_scm_closed. If you wanted to know what embedded_scm_closed means, you would look at the rule for it. The rule states: embedded_scm_closed: embedded_scm_bare | scm_function_call_closed ; Which makes very much clear what the difference of embedded_scm_closed as opposed to embedded_scm_bare is: it allows scm_function_call_closed as one additional option. Following that, you are lead to function_arglist_closed as next trivial reference, and then there is a comment * function_arglist* / function_arglist_closed*: The closed variants * don't end in simple music expressions that might still accept * things like a duration or a postevent. The closed expressions exist because of parser ambiguities and are slated to go eventually, but while they are around, their meaning _is_ documented, and you get to the documentation by following the obvious track. The commit message might be misleading, however, since it sounds like it is touting a fundamental new feature. What this is is more like putting another piece in place for the general idea of strings/markups can be submitted like any Scheme expression would. It is more like plugging an oversight rather than adding something new. I had expected this usage to have worked already before submitting this patch, and this patch is supposed to make LilyPond match this expectation stemming from earlier work apparently still incomplete. So I might need to reword the commit message. http://codereview.appspot.com/6812088/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Allow (closed) scheme function calls as text scripts. (issue 6812088)
Very nice! http://codereview.appspot.com/6812088/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow (closed) scheme function calls as text scripts. (issue 6812088)
On 2012/11/05 22:40:36, lemzwerg wrote: Very nice! Can't review the code. But from description: GREAT http://codereview.appspot.com/6812088/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow (closed) scheme function calls as text scripts. (issue 6812088)
Reviewers: lemzwerg, thomasmorley65,
Message:
On 2012/11/05 22:45:06, thomasmorley65 wrote:
On 2012/11/05 22:40:36, lemzwerg wrote:
Very nice!
Can't review the code.
But from description:
GREAT
It's actually more a fix of a shortcoming. Scheme functions should have
worked in most places for strings and markups already, but apparently I
had some places not covered, text scripts being one of them.
Description:
Allow (closed) scheme function calls as text scripts.
This allows, for example, things like
arrow = #(define-scheme-function (parser location arg) (pair?)
#{ \markup { \line { \draw-line #arg \arrow-head #X #RIGHT ##t } }
#})
\score {
\new Staff {
c4_\arrow #'(10 . 0)
}
}
which, as contrasted to markup commands, is more compact to use.
Please review this at http://codereview.appspot.com/6812088/
Affected files:
M lily/parser.yy
Index: lily/parser.yy
diff --git a/lily/parser.yy b/lily/parser.yy
index
eb8f8cbcc9c57ec85166ab5ad73011152699b90c..4eb488a37f53fed2e4a34e3a944b629e3352c49f
100644
--- a/lily/parser.yy
+++ b/lily/parser.yy
@@ -2774,7 +2774,7 @@ gen_text_def:
make_simple_markup ($1));
$$ = t-unprotect ();
}
- | embedded_scm_bare
+ | embedded_scm_closed
{
Music *m = unsmob_music ($1);
if (m m-is_mus_type (post-event))
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
