This is the mail archive of the
mailing list for the GDB project.
Re: [RFA] Implement -break-commands
On Monday 27 July 2009 Tom Tromey wrote:
> >>>>> "Volodya" == Vladimir Prus <email@example.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
> 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?