Re: [PATCH 2/3] Matching tokens: C parts (v2)

[David Malcolm <dmalcolm@redhat.com>]

On Fri, 2017-08-04 at 12:09 -0600, Jeff Law wrote:
> On 08/04/2017 08:32 AM, David Malcolm wrote:
> > On Thu, 2017-08-03 at 11:34 -0600, Jeff Law wrote:
> > > On 08/01/2017 02:21 PM, David Malcolm wrote:
> > > > Changed in v2:
> > > > 
> > > > * Renamed template argument to traits_t; eliminated subclasses,
> > > > just
> > > >   using traits struct.
> > > > * Moved enum constants into struct bodies (string constants
> > > > can't
> > > > be
> > > >   without constexpr, which isn't available in C++98).
> > > > * Fixed typo.
> > > > 
> > > > OK for trunk?
> > > > 
> > > > gcc/c/ChangeLog:
> > > > 	* c-parser.c (c_parser_error): Rename to...
> > > > 	(c_parser_error_richloc): ...this, making static, and
> > > > adding
> > > > 	"richloc" parameter, passing it to the c_parse_error
> > > > call,
> > > > 	rather than calling
> > > > c_parser_set_source_position_from_token.
> > > > 	(c_parser_error): Reintroduce, reimplementing in terms
> > > > of the
> > > > 	above, converting return type from void to bool.
> > > > 	(class token_pair): New class.
> > > > 	(struct matching_paren_traits): New struct.
> > > > 	(matching_parens): New typedef.
> > > > 	(struct matching_brace_traits): New struct.
> > > > 	(matching_braces): New typedef.
> > > > 	(get_matching_symbol): New function.
> > > > 	(c_parser_require): Add param MATCHING_LOCATION, using
> > > > it to
> > > > 	highlight matching "opening" tokens for missing
> > > > "closing"
> > > > tokens.
> > > > 	(c_parser_skip_until_found): Likewise.
> > > > 	(c_parser_static_assert_declaration_no_semi): Convert
> > > > explicit
> > > > 	parsing of CPP_OPEN_PAREN and CPP_CLOSE_PAREN to use of
> > > > 	class matching_parens, so that the pertinent open
> > > > parenthesis
> > > > is
> > > > 	highlighted when there are problems locating the close
> > > > 	parenthesis.
> > > > 	(c_parser_struct_or_union_specifier): Likewise.
> > > > 	(c_parser_typeof_specifier): Likewise.
> > > > 	(c_parser_alignas_specifier): Likewise.
> > > > 	(c_parser_simple_asm_expr): Likewise.
> > > > 	(c_parser_braced_init): Likewise, for matching_braces.
> > > > 	(c_parser_paren_condition): Likewise, for
> > > > matching_parens.
> > > > 	(c_parser_switch_statement): Likewise.
> > > > 	(c_parser_for_statement): Likewise.
> > > > 	(c_parser_asm_statement): Likewise.
> > > > 	(c_parser_asm_operands): Likewise.
> > > > 	(c_parser_cast_expression): Likewise.
> > > > 	(c_parser_sizeof_expression): Likewise.
> > > > 	(c_parser_alignof_expression): Likewise.
> > > > 	(c_parser_generic_selection): Likewise.
> > > > 	(c_parser_postfix_expression): Likewise for cases
> > > > RID_VA_ARG,
> > > > 	RID_OFFSETOF, RID_TYPES_COMPATIBLE_P, RID_AT_SELECTOR,
> > > > 	RID_AT_PROTOCOL, RID_AT_ENCODE, reindenting as
> > > > necessary.
> > > > 	In case CPP_OPEN_PAREN, pass loc_open_paren to the
> > > > 	c_parser_skip_until_found call.
> > > > 	(c_parser_objc_class_definition): Use class
> > > > matching_parens as
> > > > 	above.
> > > > 	(c_parser_objc_method_decl): Likewise.
> > > > 	(c_parser_objc_try_catch_finally_statement): Likewise.
> > > > 	(c_parser_objc_synchronized_statement): Likewise.
> > > > 	(c_parser_objc_at_property_declaration): Likewise.
> > > > 	(c_parser_oacc_wait_list): Likewise.
> > > > 	(c_parser_omp_var_list_parens): Likewise.
> > > > 	(c_parser_omp_clause_collapse): Likewise.
> > > > 	(c_parser_omp_clause_default): Likewise.
> > > > 	(c_parser_omp_clause_if): Likewise.
> > > > 	(c_parser_omp_clause_num_threads): Likewise.
> > > > 	(c_parser_omp_clause_num_tasks): Likewise.
> > > > 	(c_parser_omp_clause_grainsize): Likewise.
> > > > 	(c_parser_omp_clause_priority): Likewise.
> > > > 	(c_parser_omp_clause_hint): Likewise.
> > > > 	(c_parser_omp_clause_defaultmap): Likewise.
> > > > 	(c_parser_oacc_single_int_clause): Likewise.
> > > > 	(c_parser_omp_clause_ordered): Likewise.
> > > > 	(c_parser_omp_clause_reduction): Likewise.
> > > > 	(c_parser_omp_clause_schedule): Likewise.
> > > > 	(c_parser_omp_clause_num_teams): Likewise.
> > > > 	(c_parser_omp_clause_thread_limit): Likewise.
> > > > 	(c_parser_omp_clause_aligned): Likewise.
> > > > 	(c_parser_omp_clause_linear): Likewise.
> > > > 	(c_parser_omp_clause_safelen): Likewise.
> > > > 	(c_parser_omp_clause_simdlen): Likewise.
> > > > 	(c_parser_omp_clause_depend): Likewise.
> > > > 	(c_parser_omp_clause_map): Likewise.
> > > > 	(c_parser_omp_clause_device): Likewise.
> > > > 	(c_parser_omp_clause_dist_schedule): Likewise.
> > > > 	(c_parser_omp_clause_proc_bind): Likewise.
> > > > 	(c_parser_omp_clause_uniform): Likewise.
> > > > 	(c_parser_omp_for_loop): Likewise.
> > > > 	(c_parser_cilk_clause_vectorlength): Likewise.
> > > > 	(c_parser_cilk_clause_linear): Likewise.
> > > > 	(c_parser_transaction_expression): Likewise.
> > > > 	* c-parser.h (c_parser_require): Add param
> > > > matching_location
> > > > with
> > > > 	default UNKNOWN_LOCATION.
> > > > 	(c_parser_error): Convert return type from void to
> > > > bool.
> > > > 	(c_parser_skip_until_found): Add param
> > > > matching_location with
> > > > 	default UNKNOWN_LOCATION.
> > > > 
> > > > gcc/testsuite/ChangeLog:
> > > > 	* gcc.dg/unclosed-init.c: New test case.
> > > 
> > > Phew.  I only spot-checked most of the changes around the new API
> > > for
> > > requiring the open/close paren/brace/bracket or consuming
> > > parens/braces/brackets.  They were very mechanical :-)
> > 
> > Thanks for looking at this.  Do you have an opinion on Trevor's
> > idea
> > the the "parser" argument should be moved into the token_pair class
> > (to
> > avoid manually passing it in everywhere), or should it be kept
> > outside
> > and passed in as needed? 
> 
> No opinion.  I'd put it where ever it makes the  most logical sense
> in
> terms of code readability -- until such a point as it's shown to be a
> bottleneck.

Jason said (for the C++ part of the patch):
> About passing parser in or not, I'm happy with the current approach; 
> adding things to the stack isn't free in a highly recursive program 
> like GCC.
(in https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00535.html )

Jeff: is the C patch OK for trunk (assuming the rest of the kit is
approved), or were you just reporting that you spot-checked the
mechanical changes and that you hadn't seen any problems with those?

[...]

Thanks
Dave