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

[Han-Wen Nienhuys]

On Sun, May 2, 2010 at 8:04 AM,  <address@hidden> wrote:
>
> http://codereview.appspot.com/969046/diff/7001/8002
> File lily/lexer.ll (right):
>
> http://codereview.appspot.com/969046/diff/7001/8002#newcode545
> lily/lexer.ll:545: // loop will be EXPECT_NO_MORE_ARGS.
> On 2010/05/01 19:56:08, hanwenn wrote:
>>
>> wouldnt it be clearer to have a function
>
>>   void translate_markup_signature(SCM predicate_list,
>>      vector<int> expect_tokens);
>
>> which generates the [expect_scm, expect_scm, expect_markup] (possibly
>
> with
>>
>> expect_no_more_args) in a vector, and then push the tokens one by one
>
> in the
>>
>> desired order?
>
> 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

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.

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, so
I think a state machine is overkill.

(please enlighten if I am missing some hidden functionality that is
not clear from your code)

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.

>> Also, I think it is better to not use fall-through switches, as they
>
> are a
>>
>> uncommon and tricky idiom.
>
> Sigh.  We have essentially a state machine where the state is determined
> by the kind of already processed tokens, and depending on the state of
> the current token, we transition to a new state with different allowed
> new tokens/transitions.
>
> This sort of state machine can usefully only programmed using goto
> (unless one uses explicit state tables which are even less readable).
> The switch statement is basically the same as gotoizing a state machine,
> with the case labels substituting for goto labels.
>
> Most state machines (including this one) don't have a structure readily
> represented in linear structured code.
>
> Moving them into a separate procedure does not change that.
>
> My changes already make for the bulk of comment lines in this file.  So
> I decided against commenting this even more in violating of the
> established standards in the rest of the file.
>
> Instead I shifted the responsibility of checking the order of arguments
> to definition time instead of runtime.  It has the advantage of
> triggering prospective errors at a more expected point of time.
>
> http://codereview.appspot.com/969046/diff/7001/8002#newcode545
> lily/lexer.ll:545: // loop will be EXPECT_NO_MORE_ARGS.
> On 2010/05/01 19:56:08, hanwenn wrote:
>>
>> wouldnt it be clearer to have a function
>
>>   void translate_markup_signature(SCM predicate_list,
>>      vector<int> expect_tokens);
>
>> which generates the [expect_scm, expect_scm, expect_markup] (possibly
>
> with
>>
>> expect_no_more_args) in a vector, and then push the tokens one by one
>
> in the
>>
>> desired order?
>
>> Also, I think it is better to not use fall-through switches, as they
>
> are a
>>
>> uncommon and tricky idiom.
>
>> I would expect something like
>
>>  if(pred == bla("markup?")
>>    token = EXPECT_MARKUP;
>>  else if (pred == bla("markup-list?")
>>    token = EXPECT_MARKUP_LIST;
>>  else if (pred == bla("scm?")
>>    token = EXPECT_SCM;
>
>
>
> Done.
>
> http://codereview.appspot.com/969046/show
>



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