Re: [RFA] Implement -break-commands

[Vladimir Prus <vladimir@codesourcery.com>]

On Monday 27 July 2009 Tom Tromey wrote:

> >>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:
> 
> This all looks reasonable to me.  I have a few style nits.
> 
> Volodya> -/* Read one line from the input stream.  If the command is an "end",
> Volodya> -   return such an indication to the caller.  If PARSE_COMMANDS is true,
> Volodya> -   strip leading whitespace (trailing whitespace is always stripped)
> Volodya> -   in the line, attempt to recognize GDB control commands, and also
> Volodya> -   return an indication if the command is an "else" or a nop.
> Volodya> -   Otherwise, only "end" is recognized.  */
>  
> Volodya> -static enum misc_command_type
> Volodya> -read_next_line (struct command_line **command, int parse_commands)
> Volodya> +char *
> Volodya> +read_next_line ()
> 
> This should be marked `static' (it is declared that way but it is
> clearer to mark the definition as well).  I think it needs a header
> comment.
> 
> Volodya>  static enum command_control_type
> Volodya> -recurse_read_control_structure (struct command_line *current_cmd)
> Volodya> +recurse_read_control_structure (char * (*read_next_line_func) (), 
> Volodya> +				struct command_line *current_cmd)
> 
> The header comment should be updated to mention the new argument.
> And if you don't mind, please fix the reference to the non-existing
> "parent_control" parameter.
> 
> Volodya> +extern struct command_line *read_command_lines_1 
> Volodya> +(char * (*read_next_line_func) (), int parse_commands);
> 
> Indentation on the 2nd line.
> 
> There's some other little style nits in the patch -- over-bracing in the
> second patch, mostly.

Is that something you want fixed? I do prefer bracing everything, since that
is safer in the long run.

> 
> Volodya> +void
> Volodya> +breakpoint_set_commands (struct breakpoint *b, struct command_line *commands)
> 
> Needs a header comment.

Do we have a policy if function should be documented in header, or implementation? It
seems many existing files have comments on implementation. 

> Volodya> +static char **mi_command_line_array;
> Volodya> +static int mi_command_line_array_cnt;
> Volodya> +static int mi_command_line_array_ptr;
> 
> [...]
> 
> Volodya> +  break_command = read_command_lines_1 (mi_read_next_line, 0);
> 
> I think this would be cleaner if read_command_lines_1 took a "user_data"
> argument and then there were no new globals.

It would be clearer, though I decided not to do that because it's not
likely we'll need another such function, and the use of globals in this
context is ugly, but safe. Shall I refactor?

Thanks,
Volodya