This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Hi Keith, I do not see a reason for the virtualization via 'parse_it' field. It is called only by do_decode_line and it is the first thing do_decode_line does. Also the direct callers of do_decode_line know the virtualized function instance. Therefore the callers of do_decode_line can just pass the result from the virtualization function, without any virtualization at all. That means 'struct ls_parser' is "parser state", not "parser definition". I have attached such simplification proof-of-concept. On Fri, 27 Jul 2012 05:46:29 +0200, Keith Seitz wrote: > --- a/gdb/linespec.c > +++ b/gdb/linespec.c > @@ -265,6 +265,12 @@ typedef struct ls_token linespec_token; > #define LS_TOKEN_STOKEN(TOK) (TOK).data.string > #define LS_TOKEN_KEYWORD(TOK) (TOK).data.keyword > > +/* A parser function definition used by do_decode_line. */ > + > +struct ls_parser; > +typedef struct symtabs_and_lines (*linespec_parse_func) (struct ls_parser *, > + void *client_data); GDB uses more *_ftype. And it is function type, not pointer to function type. > + > /* An instance of the linespec parser. */ > > struct ls_parser > @@ -293,6 +299,9 @@ struct ls_parser > /* The result of the parse. */ > struct linespec result; > #define PARSER_RESULT(PPTR) (&(PPTR)->result) > + > + /* The function to actually do the parsing. */ > + linespec_parse_func parse_it; > }; > typedef struct ls_parser linespec_parser; > > @@ -1433,6 +1442,29 @@ unexpected_linespec_error (linespec_parser *parser) > token_type_strings[token.type]); > } > > +/* Throw an undefined label error. */ > + > +static void ATTRIBUTE_NORETURN > +undefined_label_error (const char *function, const char *label) > +{ > + if (function != NULL) > + throw_error (NOT_FOUND_ERROR, > + _("No label \"%s\" defined in function \"%s\"."), > + label, function); Use tabs for indentation, not spaces. On more places in this patch. Also the indentation with spaces is missing one space so there probably formerly were tabs. > + else > + throw_error (NOT_FOUND_ERROR, > + _("No label \"%s\" defined in current function."), label); > +} > + > +/* Throw a source file not found error. */ > + > +static void ATTRIBUTE_NORETURN > +source_file_not_found_error (const char *name) > +{ > + throw_error (NOT_FOUND_ERROR, > + _("No source file named %s."), name); \"%s\", not %s. > +} > + > /* Parse and return a line offset in STRING. */ > > static struct line_offset > @@ -1587,9 +1619,8 @@ linespec_parse_basic (linespec_parser *parser) > else > { > /* We don't know what it was, but it isn't a label. */ > - throw_error (NOT_FOUND_ERROR, > - _("No label \"%s\" defined in function \"%s\"."), > - name, PARSER_RESULT (parser)->function_name); > + undefined_label_error (PARSER_RESULT (parser)->function_name, > + name); > } > > /* Check for a line offset. */ > @@ -1995,15 +2026,16 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls) > if no file is validly specified. Callers must check that. > Also, the line number returned may be invalid. */ > > -/* Parse the linespec in ARGPTR. */ > +/* Parse the linespec in DATA. */ > > static struct symtabs_and_lines > -parse_linespec (linespec_parser *parser, char **argptr) > +parse_linespec (linespec_parser *parser, void *data) > { > linespec_token token; > struct symtabs_and_lines values; > volatile struct gdb_exception file_exception; > struct cleanup *cleanup; > + char **argptr = (char **) data; const? > > /* A special case to start. It has become quite popular for > IDEs to work around bugs in the previous parser by quoting > @@ -2210,7 +2242,7 @@ linespec_state_constructor (struct linespec_state *self, > /* Initialize a new linespec parser. */ > > static void > -linespec_parser_new (linespec_parser *parser, > +linespec_parser_new (linespec_parser *parser, linespec_parse_func func, > int flags, const struct language_defn *language, > struct symtab *default_symtab, > int default_line, > @@ -2219,6 +2251,7 @@ linespec_parser_new (linespec_parser *parser, > parser->lexer.current.type = LSTOKEN_CONSUMED; > memset (PARSER_RESULT (parser), 0, sizeof (struct linespec)); > PARSER_RESULT (parser)->line_offset.sign = LINE_OFFSET_UNKNOWN; > + parser->parse_it = func; > linespec_state_constructor (PARSER_STATE (parser), flags, language, > default_symtab, default_line, canonical); > } > @@ -2261,42 +2294,35 @@ linespec_parser_delete (void *arg) > linespec_state_destructor (PARSER_STATE (parser)); > } > > -/* See linespec.h. */ > +/* Decode the linespec given in DATA with the PARSER. > + SELECT_MODE and FILTER are from decode_line_full. */ > > -void > -decode_line_full (char **argptr, int flags, > - struct symtab *default_symtab, > - int default_line, struct linespec_result *canonical, > - const char *select_mode, > - const char *filter) > +static void > +do_decode_line (linespec_parser *parser, void *data, > + const char *select_mode, const char *filter) > { > struct symtabs_and_lines result; > struct cleanup *cleanups; > - char *arg_start = *argptr; > VEC (const_char_ptr) *filters = NULL; > - linespec_parser parser; > struct linespec_state *state; > > - gdb_assert (canonical != NULL); > /* The filter only makes sense for 'all'. */ > gdb_assert (filter == NULL || select_mode == multiple_symbols_all); > gdb_assert (select_mode == NULL > || select_mode == multiple_symbols_all > || select_mode == multiple_symbols_ask > || select_mode == multiple_symbols_cancel); > - gdb_assert ((flags & DECODE_LINE_LIST_MODE) == 0); > > - linespec_parser_new (&parser, flags, current_language, default_symtab, > - default_line, canonical); > - cleanups = make_cleanup (linespec_parser_delete, &parser); > + cleanups = make_cleanup (null_cleanup, NULL); > save_current_program_space (); You can do: cleanups = save_current_program_space (); > > - result = parse_linespec (&parser, argptr); > - state = PARSER_STATE (&parser); > + result = (*parser->parse_it) (parser, data); > + state = PARSER_STATE (parser); > > - gdb_assert (result.nelts == 1 || canonical->pre_expanded); > - gdb_assert (canonical->addr_string != NULL); > - canonical->pre_expanded = 1; > + gdb_assert (result.nelts == 1 || state->canonical->pre_expanded); > + gdb_assert (state->canonical->addr_string != NULL); > + gdb_assert (result.nelts == 0 || state->canonical_names != NULL); > + state->canonical->pre_expanded = 1; > > /* Arrange for allocated canonical names to be freed. */ > if (result.nelts > 0) > @@ -2336,6 +2362,29 @@ decode_line_full (char **argptr, int flags, > do_cleanups (cleanups); > } > > +/* See linespec.h. > + All of the real work is done in do_decode_line. */ The split is incomplete, for example in this part there is nothing in linespec.h, such way one has to check the whole patchset to review just this part. > + > +void > +decode_line_full (char **argptr, int flags, > + struct symtab *default_symtab, > + int default_line, struct linespec_result *canonical, > + const char *select_mode, > + const char *filter) > +{ > + linespec_parser parser; > + struct cleanup *cleanups; > + > + gdb_assert (canonical != NULL); > + gdb_assert ((flags & DECODE_LINE_LIST_MODE) == 0); > + > + linespec_parser_new (&parser, parse_linespec, flags, current_language, > + default_symtab, default_line, canonical); > + cleanups = make_cleanup (linespec_parser_delete, &parser); > + do_decode_line (&parser, argptr, select_mode, filter); > + do_cleanups (cleanups); > +} > + > /* See linespec.h. */ > > struct symtabs_and_lines > @@ -2347,12 +2396,12 @@ decode_line_1 (char **argptr, int flags, > linespec_parser parser; > struct cleanup *cleanups; > > - linespec_parser_new (&parser, flags, current_language, default_symtab, > - default_line, NULL); > + linespec_parser_new (&parser, parse_linespec, flags, current_language, > + default_symtab, default_line, NULL); > cleanups = make_cleanup (linespec_parser_delete, &parser); > save_current_program_space (); > > - result = parse_linespec (&parser, argptr); > + result = (*parser.parse_it) (&parser, argptr); > > do_cleanups (cleanups); > return result; > @@ -2879,7 +2928,7 @@ symtabs_from_filename (const char *filename) > throw_error (NOT_FOUND_ERROR, > _("No symbol table is loaded. " > "Use the \"file\" command.")); > - throw_error (NOT_FOUND_ERROR, _("No source file named %s."), filename); > + source_file_not_found_error (filename); > } > > return result; Thanks, Jan
Attachment:
els-refactor-linespec-parsing-simplify.patch
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |