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#newcode63 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#newcode83 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#newcode88 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. https://codereview.appspot.com/244840043/ _______________________________________________ lilypond-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/lilypond-devel
