|
|
Created:
8 years, 10 months ago by dak Modified:
8 years, 7 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionMake music functions callable from Scheme
Establish issue 4422 as Patch 1. Test issue 4421 from master, review
from Patch 1.
Patch Set 1 : Rebased issue 4422 #Patch Set 2 : Patch on top of rebased issue 4422 #Patch Set 3 : Treat optional args differently, add documentation and regtest #
Total comments: 22
Patch Set 4 : Address Keith's suggestions, document stuff, minor bug fixes #
Total comments: 2
MessagesTotal messages: 27
Issue 4421 itself
Sign in to reply to this message.
Issue 4421 on top of issue 4422
Sign in to reply to this message.
Rebased issue 4422
Sign in to reply to this message.
Patch on top of rebased issue 4422
Sign in to reply to this message.
On 2015/05/29 17:15:21, dak wrote: > Patch on top of rebased issue 4422 Actually, it's not just rebased. Accessing the fluids has been encapsulated in a new class Fluid defined in lily/include/fluid.hh.
Sign in to reply to this message.
Treat optional args differently, add documentation and regtest
Sign in to reply to this message.
Well, I can't say LGTM as that would imply I'd understood it and could confirm its correctness, but I can say I'm looking forward to removing lots of the #{ ... #} in the satb.ly template and its supporting cast! Trevor
Sign in to reply to this message.
On 2015/06/03 18:29:44, Trevor Daniels wrote: > Well, I can't say LGTM as that would imply I'd understood it and could confirm > its correctness, but I can say I'm looking forward to removing lots of the #{ > ... #} in the satb.ly template and its supporting cast! > > Trevor Actually, the satb template contains a sizeable amount of stuff akin to #{ #x #} or #{ \markup #x #} which can be replaced without bother with x already. This patch here only concerns music function calls.
Sign in to reply to this message.
LGTM I gather that Scheme fluids are variables with global scope but distinct storage for each Scheme thread, so they can be used to reference to the specific parser (if any) in use by the thread that called the music function. https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tel... Documentation/changes.tely:68: as if they were proper Scheme functions. Argument checking will Skip "as if they were.." if they really are proper Scheme functions. https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh File lily/include/fluid.hh (right): https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh#new... lily/include/fluid.hh:24: "storage of Scheme fluids" so that people know you mean the Lisp/Scheme concept and not the normal meaning of 'fluid'. https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh#new... lily/include/fluid.hh:36: // where the garbage collector will be able to find it. The passive voice leaves things ambiguous. I think "declare only local Fluid variables, not statics, not arrays of Fluids, so that the Fluid object is in automatic storage where the Guile garbage collector can find it." https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh#new... lily/include/fluid.hh:44: SCM value_; // the cache https://codereview.appspot.com/244840043/diff/80001/lily/include/music-functi... File lily/include/music-function.hh (right): https://codereview.appspot.com/244840043/diff/80001/lily/include/music-functi... lily/include/music-function.hh:34: SCM call (SCM args); Wouldn't this be more conventional as a static member function ? static SCM call( SCM music_function_smob, SCM args_list);
Sign in to reply to this message.
https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tel... Documentation/changes.tely:68: as if they were proper Scheme functions. Argument checking will On 2015/06/07 22:16:00, Keith wrote: > Skip "as if they were.." if they really are proper Scheme functions. I guess the result of (define-music-function ... ) with all its type-checking and references to the source line-numbers is something more than a simple Scheme function, so I see why you say "as if they were"
Sign in to reply to this message.
It bothered me that I said 'LGTM' without figuring out the logic. I don't understand it. https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc File lily/music-function.cc (right): https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#ne... lily/music-function.cc:63: bool skipping = false; The 'skipping' flag and the early continues are hard to follow. We want to keep this logic synchronized with the duplicate functionality in the parser, so either now or later it would be good to make the logic more clear. What would be clear for me is a boolean remembering whether the argument currently being matched is optional: for ( ; signature; ++signature) if pair(car(signature)) pred = caar(signature) default = cadr(signature) or #f optional = true else pred = car(signature) optional = false if !input_arg error "too few args" return if type-predicate( input_arg ) arg = input_arg++ elseif optional arg = default_arg if *unspecified* == input_arg input_arg++ else error "wrong argument type" return append arg to argslist endfor if input_arg warn "too many args" https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#ne... lily/music-function.cc:83: skipping = true; Here we failed to type-match an optional argument, and will fill the slot with the default value on line 88, so why set the 'skipping' flag ? If there are two optional parameters in a row, the 'skipping' flag would seem to force the next entry in the signature to be filled with its default value as well. https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#ne... lily/music-function.cc:88: args = scm_cons (with_loc (scm_cdr (pred), location), args); For an optional argument with no default value, it seems we would want #f, but scm_cdr will return the empty list.
Sign in to reply to this message.
This reply is not intended to refute your contention that there are too few comments. It should rather make you understand what is actually going on so that you can improve the suggestions. Maybe even more documentation elsewhere would be needed. https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc File lily/music-function.cc (right): https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#ne... lily/music-function.cc:63: bool skipping = false; On 2015/06/08 04:36:54, Keith wrote: > The 'skipping' flag and the early continues are hard to follow. We want to keep > this logic synchronized with the duplicate functionality in the parser, so > either now or later it would be good to make the logic more clear. > > What would be clear for me is a boolean remembering whether the argument > currently being matched is optional: It doesn't do the trick. Optional arguments following an unmatched optional argument are automatically unmatched themselves. https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#ne... lily/music-function.cc:83: skipping = true; On 2015/06/08 04:36:54, Keith wrote: > Here we failed to type-match an optional argument, and will fill the slot with > the default value on line 88, so why set the 'skipping' flag ? If there are two > optional parameters in a row, the 'skipping' flag would seem to force the next > entry in the signature to be filled with its default value as well. This is _exactly_ what it does. You say "It bothered me that I said 'LGTM' without figuring out the logic. I don't understand it. We want to keep this logic synchronized with the duplicate functionality in the parser," I rather think you don't understand what the parser does. Which is hardly surprising since the parser logic is written in a manner where the rules are applied from last argument to first with the predicates coming in in reverse order and the actual tokens being read front to back. I've managed to get it streamlined somewhat over the years but it made my head hurt for weeks to get the basic control logic for optional arguments and their skipping correct. The achieved functionality is exacly what is done here. Only that this code here does it in a straightforward manner speaking for itself with the semantics clearly in the open. Given the comparative complexity of the implementation in an LALR(1) parser, I might be guilty of "simple enough to not need a comment" syndrome here. But if you are confused because this code seems to do something different than the parser does, chances are very much that it is because you understand what this code does and misunderstand how the parser does the same. Which is absolutely understandable. The only excuse for the parser doing this job as complex as it does is that it cannot really be done any simpler using the parser generator we use, and once it works, it's comparatively low-maintenance. So your problem appears to be that you understand the code but it does something you did not expect. Cross-check from the user end with the "Extending LilyPond" guide's explanation of optional arguments. https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#ne... lily/music-function.cc:88: args = scm_cons (with_loc (scm_cdr (pred), location), args); On 2015/06/08 04:36:54, Keith wrote: > For an optional argument with no default value, it seems we would want #f, but > scm_cdr will return the empty list. Signatures are not stored in the original form. They are slightly processed in order to make their use faster. scm_cdr will return #f since (number? . #f) is what will be stored for a predicate written as (number?) while (number? . 7) is what will be stored for (number? 7). Could be worth mentioning.
Sign in to reply to this message.
https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tel... Documentation/changes.tely:68: as if they were proper Scheme functions. Argument checking will On 2015/06/07 22:49:05, Keith wrote: > On 2015/06/07 22:16:00, Keith wrote: > > Skip "as if they were.." if they really are proper Scheme functions. > > I guess the result of > (define-music-function ... ) > with all its type-checking and references to the source line-numbers is > something more than a simple Scheme function, so I see why you say "as if they > were" Well, when called from Scheme there is not much in the line of "references to the source line-numbers". The result of define-music-function would now meet the predicate procedure?, had procedure-properties so it is sort of a function. But it is actually a GUILE data structure which has additional function-call semantics. And calling procedure-environment on it fails (no idea how this would behave in GUILE 2 though: it seems like allowing capture of a procedure environment could be an interesting extension). The C++ equivalent would be something like being derived from a function class (actual C++ functions are not of a type that can be used as base class but there are some comparable constructs). Maybe I should not use "proper" but "genuine" here. I don't think GUILE has an established term for this we could use.
Sign in to reply to this message.
https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh File lily/include/fluid.hh (right): https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh#new... lily/include/fluid.hh:24: On 2015/06/07 22:16:00, Keith wrote: > "storage of Scheme fluids" so that people know you mean the Lisp/Scheme concept > and not the normal meaning of 'fluid'. Probably rather "GUILE fluids" since Scheme in general does not have them. Scheme srfi-39 provides "parameters" which are somewhat similar. https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh#new... lily/include/fluid.hh:44: SCM value_; On 2015/06/07 22:16:02, Keith wrote: > // the cache If that comment would clarify matters, it would seem like I should be calling the field "cache_" in the first place. Is fluid_/cache_ better than fluid_/value_? I think the latter captures the distinction between container and content better.
Sign in to reply to this message.
On Sun, 07 Jun 2015 23:09:31 -0700, <dak@gnu.org> wrote: > So your problem appears to be that you understand the code but it does > something you did not expect. Cross-check from the user end with the > "Extending LilyPond" guide's explanation of optional arguments. It is Extending section 2.2.2. Yuck. A comment pointing to Extending-2.2.2 would help. The behavior in the parser is forced because LilyPond uses no delimiters for the arguments to a function. Scheme has delimiters so you know where the end of the argument list is, so we could use a simpler rule for optional arguments in Scheme interface. Or you could scan arguments using a recursive-descent method in C, so the parallel with the parser is maybe a bit more clear. Or maybe advance through 'signature' to the next non-optional argument immediately upon using a default value, rather than setting 'skipping'. The re-structuring I suggested was intended also to change the if-else branching so that you don't need the 'continue's. > https://codereview.appspot.com/244840043/
Sign in to reply to this message.
On 2015/06/08 15:58:31, Keith wrote: > On Sun, 07 Jun 2015 23:09:31 -0700, <mailto:dak@gnu.org> wrote: > > > So your problem appears to be that you understand the code but it does > > something you did not expect. Cross-check from the user end with the > > "Extending LilyPond" guide's explanation of optional arguments. > > It is Extending section 2.2.2. Yuck. A comment pointing to Extending-2.2.2 > would help. > > The behavior in the parser is forced because LilyPond uses no delimiters for the > arguments to a function. Scheme has delimiters so you know where the end of the > argument list is, so we could use a simpler rule for optional arguments in > Scheme interface. I did in version 2 of the patch set. Then I reconsidered. The reason I reconsidered is that extending some LilyPond command often may involve adding obscure optional arguments, like context modifications. Users will rarely use those, and they don't disturb existing usage when their type has no overlap with the type of the argument following it. So if we use a strictly positional interface in Scheme calls, people will not be able to translate their LilyPond function calls into Scheme function calls with confidence. When I converted about 5 trivial #{ \... #} calls in ly/music-functions-init.ly to Scheme, I was worried I may have forgot about some optional argument I would have needed to specify. This seed of doubt and lack of forward-compatibility of functions that don't yet have an optional argument does not seem like it would be doing anybody a favor. > Or you could scan arguments using a recursive-descent method in C, so the > parallel with the parser is maybe a bit more clear. Sorry, but no. This implementation follows the plain language explanation in the documentation. And it does this well enough that you picked up what it did from the code without much apparent difficulty and complained that it did not match what you thought the code in the parser did. So you want to have code that you understood changed to match code you did not understand. That's not going to help anybody. If anything, it should be the parser code that should be matched to the C code in order to become comprehensible, but that's basically out of our hand since we are limited by the expressivity of the Bison grammar. The parser has extensive comments laying out the basic problem of the argument parsing and how it is expressed in the grammar. That's not the same as having the actual grammar/code being simple and straightforward but it's the best we can do as I see it. > Or maybe advance through 'signature' to the next non-optional argument > immediately upon using a default value, rather than setting 'skipping'. I thought about that, yes. It would give us one outer loop that goes through actual argument list and signature in lockstep, and one inner loop that goes through signature only. I think that I tried this already and that it did not really work out well. I'll give it another attempt but I doubt I'll come up with something better. > The re-structuring I suggested was intended also to change the if-else branching > so that you don't need the 'continue's. I prefer early continue, break and return statements to nested two-way conditionals. A "continue;" clearly spells out "end of loop, you don't need to further think about this code branch" while "} else {" can still have relevant loop parts following after the matching "}". So reducing the number of "continue" statements is definitely not an attraction for me. Reducing duplicated code before such statements might be somewhat more interesting.
Sign in to reply to this message.
On 2015/06/08 16:38:28, dak wrote: > On 2015/06/08 15:58:31, Keith wrote: > > Or maybe advance through 'signature' to the next non-optional argument > > immediately upon using a default value, rather than setting 'skipping'. > > I thought about that, yes. It would give us one outer loop that goes through > actual argument list and signature in lockstep, and one inner loop that goes > through signature only. I think that I tried this already and that it did not > really work out well. I'll give it another attempt but I doubt I'll come up > with something better. No dice. The main problem is dealing with all combinations of running out of signature and running out of available arguments. If you treat the optional argument skipping in an inner loop, you may still run out of signature, and in that case you still need to treat the case where you have simultaneously run out of arguments (namely because the non-matching argument triggering the skipping was \default aka *unspecified) separately _and_ break out of all loops. Since you have already fetched the next signature bit (since you would not know whether it was optional and skipping should continue), you can't return to the main loop which would fetch it again but need to clad the whole in one while (scm_is_false (scm_call_1 (pred, arg))) of its own. Treating all the terminating condition combinations in inner loop and outer loop separately causes the control structures to fray apart more than the "skipping" flag does, to the degree where the cleanest solution to handle one of the innermost loop termination conditions with a goto statement. I've spent another whole work day on this. Feel free to invest time of your own on a followup issue/patch better matching your code aesthetics. The regtest, while not complete coverage, does a good job of exercising most of the code paths in question.
Sign in to reply to this message.
This mainly addresses the points of Keith's critique where feasible. Several proposals were inadvisable or based on wrong assumptions: I've put in extensive documentation in order to make this more obvious. Apart from slight code arrangement and lots of documentation, I also removed functionality from the Fluid class that has shoot-yourself-in-the-foot potential, and I fixed a slight error in the fallback for printing of notenames without accessible parser. https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tel... Documentation/changes.tely:68: as if they were proper Scheme functions. Argument checking will On 2015/06/08 09:17:27, dak wrote: > On 2015/06/07 22:49:05, Keith wrote: > > On 2015/06/07 22:16:00, Keith wrote: > > > Skip "as if they were.." if they really are proper Scheme functions. > > > > I guess the result of > > (define-music-function ... ) > > with all its type-checking and references to the source line-numbers is > > something more than a simple Scheme function, so I see why you say "as if > they > > were" > > Well, when called from Scheme there is not much in the line of "references to > the source line-numbers". The result of define-music-function would now meet > the predicate procedure?, had procedure-properties so it is sort of a function. > But it is actually a GUILE data structure which has additional function-call > semantics. And calling procedure-environment on it fails (no idea how this > would behave in GUILE 2 though: it seems like allowing capture of a procedure > environment could be an interesting extension). The C++ equivalent would be > something like being derived from a function class (actual C++ functions are not > of a type that can be used as base class but there are some comparable > constructs). > > Maybe I should not use "proper" but "genuine" here. I don't think GUILE has an > established term for this we could use. Done. https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh File lily/include/fluid.hh (right): https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh#new... lily/include/fluid.hh:24: On 2015/06/08 09:32:06, dak wrote: > On 2015/06/07 22:16:00, Keith wrote: > > "storage of Scheme fluids" so that people know you mean the Lisp/Scheme > concept > > and not the normal meaning of 'fluid'. > > Probably rather "GUILE fluids" since Scheme in general does not have them. > Scheme srfi-39 provides "parameters" which are somewhat similar. Done. https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh#new... lily/include/fluid.hh:36: // where the garbage collector will be able to find it. On 2015/06/07 22:16:02, Keith wrote: > The passive voice leaves things ambiguous. I think "declare only local Fluid > variables, not statics, not arrays of Fluids, so that the Fluid object is in > automatic storage where the Guile garbage collector can find it." Acknowledged. I replaced the whole paragraph instead. https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh#new... lily/include/fluid.hh:44: SCM value_; On 2015/06/08 09:32:06, dak wrote: > On 2015/06/07 22:16:02, Keith wrote: > > // the cache > > If that comment would clarify matters, it would seem like I should be calling > the field "cache_" in the first place. Is fluid_/cache_ better than > fluid_/value_? I think the latter captures the distinction between container > and content better. Acknowledged. Added a more verbose comment. https://codereview.appspot.com/244840043/diff/80001/lily/include/music-functi... File lily/include/music-function.hh (right): https://codereview.appspot.com/244840043/diff/80001/lily/include/music-functi... lily/include/music-function.hh:34: SCM call (SCM args); On 2015/06/07 22:16:02, Keith wrote: > Wouldn't this be more conventional as a static member function ? > static SCM call( SCM music_function_smob, SCM args_list); LY_DECLARE_SMOB_PROC works with non-static member function pointers. The reasons for that were reviewed separately when it was introduced. Not relevant for reviewing this issue. https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc File lily/music-function.cc (right): https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#ne... lily/music-function.cc:83: skipping = true; On 2015/06/08 04:36:54, Keith wrote: > Here we failed to type-match an optional argument, and will fill the slot with > the default value on line 88, so why set the 'skipping' flag ? If there are two > optional parameters in a row, the 'skipping' flag would seem to force the next > entry in the signature to be filled with its default value as well. I've added extensive documentation and references to make clear this is correct. https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#ne... lily/music-function.cc:88: args = scm_cons (with_loc (scm_cdr (pred), location), args); On 2015/06/08 06:09:31, dak wrote: > On 2015/06/08 04:36:54, Keith wrote: > > For an optional argument with no default value, it seems we would want #f, but > > scm_cdr will return the empty list. > > Signatures are not stored in the original form. They are slightly processed in > order to make their use faster. scm_cdr will return #f since (number? . #f) is > what will be stored for a predicate written as (number?) while (number? . 7) is > what will be stored for (number? 7). > > Could be worth mentioning. Done.
Sign in to reply to this message.
Address Keith's suggestions, document stuff, minor bug fixes
Sign in to reply to this message.
Just querying a strange sentence. https://codereview.appspot.com/244840043/diff/100001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/244840043/diff/100001/Documentation/changes.te... Documentation/changes.tely:69: matching based will still be performed in the same manner as when "Argument checking and matching based will still ..." ?? Is something missing here?
Sign in to reply to this message.
https://codereview.appspot.com/244840043/diff/100001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/244840043/diff/100001/Documentation/changes.te... Documentation/changes.tely:69: matching based will still be performed in the same manner as when On 2015/06/09 16:26:23, Trevor Daniels wrote: > "Argument checking and matching based will still ..." ?? > Is something missing here? I rather think that "based" survived from an earlier version. I'll delete it but will not upload a new patch for this single-word change.
Sign in to reply to this message.
LGTM The comments make it make sense.
Sign in to reply to this message.
On Mon, 08 Jun 2015 18:04:44 -0700, <dak@gnu.org> wrote: > No dice. The main problem is dealing with all combinations of running > out of signature and running out of available arguments. If you treat > the optional argument skipping in an inner loop, you may still run out > of signature, and in that case you still need to treat the case where > you have simultaneously run out of arguments (namely because the > non-matching argument triggering the skipping was \default aka > *unspecified) separately _and_ break out of all loops. Since you have > already fetched the next signature bit (since you would not know whether > it was optional and skipping should continue), you can't return to the > main loop which would fetch it again but need to clad the whole in one > while (scm_is_false (scm_call_1 (pred, arg))) of its own. Treating all > the terminating condition combinations in inner loop and outer loop > separately causes the control structures to fray apart more than the > "skipping" flag does, to the degree where the cleanest solution to > handle one of the innermost loop termination conditions with a goto > statement. Well, the inner loop can decide whether to continue by peeking-ahead at the next entry in the signature. Then the jobs of the inner/outer loops remain clean. Some people don't like side-effects in loop-control conditionals while (scm_is_pair (scm_cdr (signature)) // more slots ? && scm_is_pair (scm_cadr (signature)) // next one optional ? && (signature = scm_cdr (signature)));// deal with it now. so maybe I'll use test-in-the-middle loop, but that needs only break one loop.
Sign in to reply to this message.
On Mon, 08 Jun 2015 18:04:44 -0700, <dak@gnu.org> wrote: > No dice. The main problem is dealing with all combinations of running > out of signature and running out of available arguments. If you treat > the optional argument skipping in an inner loop, you may still run out > of signature, and in that case you still need to treat the case where > you have simultaneously run out of arguments (namely because the > non-matching argument triggering the skipping was \default aka > *unspecified) separately _and_ break out of all loops. Since you have > already fetched the next signature bit (since you would not know whether > it was optional and skipping should continue), you can't return to the > main loop which would fetch it again but need to clad the whole in one > while (scm_is_false (scm_call_1 (pred, arg))) of its own. Treating all > the terminating condition combinations in inner loop and outer loop > separately causes the control structures to fray apart more than the > "skipping" flag does, to the degree where the cleanest solution to > handle one of the innermost loop termination conditions with a goto > statement. Well, the inner loop can decide whether to continue by peeking-ahead at the next entry in the signature. Then the jobs of the inner/outer loops remain clean. Some people don't like side-effects in loop-control conditionals while (scm_is_pair (scm_cdr (signature)) // more slots ? && scm_is_pair (scm_cadr (signature)) // next one optional ? && (signature = scm_cdr (signature)));// deal with it now. so maybe I'll use test-in-the-middle loop, but that needs only break one loop. https://codereview.appspot.com/247190043/
Sign in to reply to this message.
"Keith OHara" <k-ohara5a5a@oco.net> writes: > On Mon, 08 Jun 2015 18:04:44 -0700, <dak@gnu.org> wrote: > >> No dice. The main problem is dealing with all combinations of running >> out of signature and running out of available arguments. If you treat >> the optional argument skipping in an inner loop, you may still run out >> of signature, and in that case you still need to treat the case where >> you have simultaneously run out of arguments (namely because the >> non-matching argument triggering the skipping was \default aka >> *unspecified) separately _and_ break out of all loops. Since you have >> already fetched the next signature bit (since you would not know whether >> it was optional and skipping should continue), you can't return to the >> main loop which would fetch it again but need to clad the whole in one >> while (scm_is_false (scm_call_1 (pred, arg))) of its own. Treating all >> the terminating condition combinations in inner loop and outer loop >> separately causes the control structures to fray apart more than the >> "skipping" flag does, to the degree where the cleanest solution to >> handle one of the innermost loop termination conditions with a goto >> statement. > > Well, the inner loop can decide whether to continue by peeking-ahead > at the next entry in the signature. Then it needs to separately check that it can peek ahead. If I'm not mistaken, I do that in several places as well in the current single loop. Doing it in a double loop makes this moderately more confusing. > Then the jobs of the inner/outer loops remain clean. > > Some people don't like side-effects in loop-control conditionals > > while (scm_is_pair (scm_cdr (signature)) // more slots ? > && scm_is_pair (scm_cadr (signature)) // next one optional ? > && (signature = scm_cdr (signature)));// deal with it now. Count me among "some people" here. At any rate, this relies on SCM being equivalent to "true" in a boolean context which is not something we can rely on. So you'd rather need something like && ((signature = scm_cdr (signature)), true) here and that's where we are seriously in "ugh" territory at the latest. > so maybe I'll use test-in-the-middle loop, Some people don't like test-in-the-middle loop either, but I usually prefer it to the alternatives. > but that needs only break one loop. Well, I'm sympathetic to your argument in theory, and I have an (unfinished) branch issue4421-keith to show for it with a day of work behind it. But I painted myself into a corner with it that became uglier than what it was supposed to replace. I am pushing that branch (unrebased though) to dev/dak/issue4421-keith so that you have the option of saving yourself that day or part of it but of course if you choose to base your work on that code, you have a solid chance of sticking in the same corner whereas starting from scratch you might find something better that evaded me. Take your choice. Mine was to take my losses with the first approach and leave. It was certainly good enough and I was satisfied I could not do significantly better with reasonable effort. -- David Kastrup
Sign in to reply to this message.
On Sat, 20 Jun 2015 22:28:13 -0700, David Kastrup <dak@gnu.org> wrote: > "Keith OHara" <k-ohara5a5a@oco.net> writes: > >> Some people don't like side-effects in loop-control conditionals >> >> while (scm_is_pair (scm_cdr (signature)) // more slots ? >> && scm_is_pair (scm_cadr (signature)) // next one optional ? >> && (signature = scm_cdr (signature)));// deal with it now. > > Count me among "some people" here. Me too. A minute after I sent this I thought of a simpler way at https://codereview.appspot.com/247190043/
Sign in to reply to this message.
Message was sent while issue was closed.
On 2015/06/07 22:16:02, Keith wrote: > LGTM > > I gather that Scheme fluids are variables with global scope but distinct storage > for each Scheme thread, so they can be used to reference to the specific parser > (if any) in use by the thread that called the music function. Scheme fluids are not as much "variables" as they are values you can assign to variables and other data structures. Their value is associated with dynamic scope (dynamic scope is what "catch" expressions for exceptions have: they run out of validity the moment you leave their continuation but are available anywhere within, independent of lexical scope). In order not to make things too confusing, most fluids are indeed stored in global variables and accessed from there. Sorry for answering this a bit late.
Sign in to reply to this message.
|