Re: Don't hardcode a limited set of markup signatures. (issue969046)

[Han-Wen Nienhuys]

On Sun, May 2, 2010 at 2:14 PM, David Kastrup <address@hidden> wrote:
> Han-Wen Nienhuys <address@hidden> writes:
>
>> On Sun, May 2, 2010 at 8:04 AM,  <address@hidden> wrote:
>>>
>>> The tokens _are_ pushed one by one in the desired order.  So it makes
>>> no sense to allocate additional storage just to do the same job.
>>
>> This is not about saving storage. This block of code does 2 things:
>>
>> - translation between predicates and token numbers
>> - pushing the tokens onto the stack
>
> That's what the _current_ implementation, Patch set 4, does.  Have you
> looked at it?  Why do you keep complaining about code I already removed
> before replying?
>
> The implementation you complained about did more than you claim: it also
> made sure that the order of arguments was Scheme arguments first, markup
> arguments second, markup list arguments last.
>
> It's not like that was not _extensively_ documented.
>
>> ideally, this would be broken up in two separate blocks of code, so we
>> dont have one 70 line block of code, which suggests that something
>> much more complicated is going on.
>

I apologise - I opened the page again, and expected it to always show
the latest patch set, which it didn't obviously. That, coupled with a
lengthy explanation of about using state machines made assume
erroneously that you did not change the .ll code

The lexer code looks OK now - you could remove the markup_p variables;
ly_module_constant() memoizes the lookup, so the second lookup will
basically be a noop.

As for the scheme code, it would be nice if the validity check were
moved in a separate pure function, so the setter does

(define (set-signature cmd sig)
  (if (valid-sig? sig) (set-property .. .)
      (ly:error (_ "invalid signature for command)))

> Read the comments if you don't feel you can guess from the code.  The
> comments (to a good degree Patch set 3, which you asked for but
> apparently did not look at closely, quite extensively explained what was
> done and why).
>
>> Why are you creating a state machine to apply a predicate=>token map
>> over a list?  This mapping is independent of the last pushed token
>
> Wrong.  When the only allowed predicate at the current point in the
> argument list is markup-list?, there is no point establishing whether
> the current predicate is markup? or something else.  I need not
> distinguish between different invalid predicates.  Depending on the
> position in the argument list, there is no necessity for a complete
> mapping.
>
>> , so I think a state machine is overkill.
>>
>> (please enlighten if I am missing some hidden functionality that is
>> not clear from your code)
>
> It established that the order of arguments is correct.  For establishing
> correct sequencing, a state machine is the way to go.  And this
> particular state machine was boiled down to very concise code, doing
> exactly what the comments said it did.
>
> But this is academical as I _removed_ all that code already.  It is no
> longer established at call time whether the signature is valid.  That
> check has been moved to _definition_ time in markup.scm.  lexer.ll
> should now only contain code that you can understand immediately without
> bothering to look at the comments.
>
> I have no doubts that the changes in markup.scm will not meet your
> approval, either.  Feel free to write this differently.
>
>> The quality of the current code is not an argument to not improve over
>> it.  The current code is largely from my hand, but there are many
>> things I would do differently today for many reasons.
>
> In the time you spent complaining about code I already removed prior to
> your complaint, you could have done some things differently.  Or at
> least complained about the current code.
>
> It does not appear that the Rietveld review process is used for more
> than annoying would-be contributors.
>
> In this case, quite successfully.
>
> Likely the best I can do is file a bug report about the markup command
> limitations and include a pointer to the patch set, explaining that it
> can't be accepted in its current form because of its descent from
> unworthy code.

For one reason or another, whenever I review code from you it degrades
into a fight. I am not quite sure why this always happens.

Could we mutually assume that when the other party makes comments or
writes improvements, that all of this happens in good faith, so if
something seems wrong, we assume the other party made an innocent
mistake?

thank you.

-- 
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen