Hollis Blanchard <[EMAIL PROTECTED]> writes:
> I don't have many comments, since I'm not familiar with lexing and
> parsing. Just a couple small comments:
>
> On Oct 29, 2005, at 4:41 PM, Marco Gerards wrote:
>>
>> `if' accepts any command. I showed the example of using the `['
>> command. It can be made similar to the `[' command of coreutils.
>> This command sets the environment variable `RESULT' to either 0 or 1.
>> After execution `if' checks `RESULT' and either executes the `if' or
>> the `else' branch. It would also be possible to use a return value
>> instead of `RESULT', but that was too much trouble for me at first.
>> But of course this can be changed if someone can convince me of that.
>
> You want it to be "$RESULT" instead of "$?" ?
You are right.
> I really don't like that each command has to explicitly set RESULT. As
> you note, it would be better if the return code from the command were
> automatically placed into the status environment variable.
Most command return grub_err_t. The only commands that matter for us
are commands like `['. Would you propose every commands returns an
int and that on function return grub_errno is checked?
>> +#ifdef GRUB_UTIL
>> +void
>> +grub_lsb_init (void)
>> +{
>> + grub_register_command ("[", grub_cmd_lsb, GRUB_COMMAND_FLAG_CMDLINE,
>> + "[ EXPRESSION ]", "Evaluate an expression", 0);
>> +}
>> +
>> +void
>> +grub_lsb_fini (void)
>> +{
>> + grub_unregister_command ("[");
>> +}
>> +#else /* ! GRUB_UTIL */
>> +GRUB_MOD_INIT
>> +{
>> + (void)mod; /* To stop warning. */
>> + grub_register_command ("[", grub_cmd_lsb, GRUB_COMMAND_FLAG_CMDLINE,
>> + "[ EXPRESSION ]", "Evaluate an expression", 0);
>> +}
>> +
>> +GRUB_MOD_FINI
>> +{
>> + grub_unregister_command ("[");
>> +}
>> +#endif /* ! GRUB_UTIL */
>
> We *really* need to redefine GRUB_MOD_INIT/FINI to remove all this
> duplicated code. I guess I will add that to my list.
Cool :-)
>> +/* A part of an argument. */
>> +struct grub_script_arg
>> +{
>> + /* If this is 0, STR is a string. If it is one, STR is a variable
>> + name. */
>> + int type;
>
> This should probably be an enum.
Yes, good catch.
> Since this code won't break any existing behavior (simple "ls (hd,0)/"
> will still work, right?), I guess it can be committed as soon as
> serious issues like the memory leak have been fixed.
That is right. All normal code will continue to function, otherwise
it would be a bug or have been discussed on the list. So far there is
nothing that would break the current behavior except multiple lines
(which I should fix before committing).
--
Marco
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
