Re: [PATCH] Bug fix for parser

[Marco Gerards <mgerards@xs4all.nl>]

Bean <bean123ch@gmail.com> writes:

>> > if grub_error is set, it will effect the parser, caused menu not to be
>> > showed, etc.
>>
>> Right, but do you want to see a menu if not everything is correctly executed?
>
> the problem is that it's not always an error when grub_errno is set,
> for example, the test command use it to report whether or not two
> strings equals.

Agreed.

>> >> I do not like the name "script_1".
>> >
>> > what do you suggest ?
>>
>> Hopefully someone else knows a better name? :-)
>
> how about script_init for the old script, and script here.

Fine for me.  Besides that, we can always change this and in the
meanwhile these names will do.

>> >> >  commandblock:        '{'
>> >> >                 {
>> >> >                   grub_script_lexer_ref (state->lexerstate);
>> >> > -                    grub_script_lexer_record_start (state->lexerstate);
>> >>
>> >> Ehm?  Can you explain this?  If I am not mistaken, this will result in
>> >> a memory leak.
>> >
>> > I split commandblock from menuentry, now it's a standalone statement.
>>
>> Ah ok, so there is a grub_script_lexer_record_start elsewhere that
>> forfills the same role?
>
> yes, the grub_script_lexer_record_start is in menuentry.

Good :-)

>> >> What was the problem here?
>> >
>> > I don't like the idea of deref in another statement, and now we can
>> > use {} separately.
>>
>> Of deref?  Can you explain?  The idea of commandblock was that this
>> structure might show up more often.  But I have no objections for now
>> if a bug is fixed this way.
>
> in the old implementation,
>
> commandblock:	'{'
> 		  {
> 		    grub_script_lexer_ref (state->lexerstate);
> 		  }
>                 newlines commands '}'
>                   {
> 		    grub_script_lexer_deref (state->lexerstate);
>                     grub_script_lexer_record_start (state->lexerstate);
> 		    $$ = $4;
> 		  }
> ;
>
> menuentry:	"menuentry" argument newlines commandblock
> 		  {
> 		    char *menu_entry;
> 		    menu_entry = grub_script_lexer_record_stop (state->lexerstate);
> 		    $$ = grub_script_create_cmdmenu (state, $2, menu_entry, 0);
> 		  }
>
> it calls grub_script_lexer_record_start in commandblock, and
> grub_script_lexer_record_stop in menuentry, but now it's both inside
> menuentry. i think it's better this way. besides, if you use
> commandblock alone in old implemenation, it will cause problem becuase
> there is no matching grub_script_lexer_record_stop.

For now your approach is ok.  But perhaps I will reintroduce this
properly some day.  We'll see :-)

>> Btw, does this patch incorporate the previous patch you sent in
>> regarding scripting?
>
> I didn't include the fix on the lexer, for example. ${AA}${BB} should
> expand  to one token instead of two. I think it's better to use a
> separate patch.

Right.  I am completely unhappy with the lexer as it is now... :-/
The main reason for writing it manually is that flex depends on all
kinds of stuff I do not want in GRUB.  Like reading files, etc.

Thanks a lot for fixing bugs related to scripting.  Lately I am rather
busy and I was the only person who worked on it.  More testing, better
error handling, etc is appriciated.

I still have an unfinished "test" command on my HD.

--
Marco