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

[David Kastrup]

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.

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.

-- 
David Kastrup