Re: [PATCH 1/5] Remove global variable tracepoint_list and stepping_list.

[Pedro Alves <palves@redhat.com>]

On 06/07/2013 02:08 PM, Yao Qi wrote:
> This patch is a refactor patch.  Simply, it removes two global
> variables tracepoint_list and stepping_list.
> 
> gdb:
> 
> 2013-06-07  Pedro Alves  <pedro@codesourcery.com>
> 	    Yao Qi  <yao@codesourcery.com>
> 
> 	* tracepoint.c (tracepoint_list, stepping_list): Remove.
> 	(do_clear_collection_list, init_collection_list): New.
> 	(encode_actions): Change the type of the second and third
> 	parameter from 'char ***' to 'struct collection_list *'.
> 	(encode_actions): Rename to encode_actions_rsp, and rewrite on top
> 	of the new encode_actions implementation.
> 	(encode_actions): New function.

Also, "Make static.".

> 	(_initialize_tracepoint): Delete references to 'tracepoint_list'
> 	and 'stepping_list'.
> 	* tracepoint.h (encode_actions): Remove declaration.
> 	(encode_actions_rsp): Add declaration.
> 	* remote.c (remote_download_tracepoint): Caller update.

>  
>  /* MEMRANGE functions: */
>  
> @@ -1239,6 +1238,35 @@ clear_collection_list (struct collection_list *list)
>    list->next_aexpr_elt = 0;
>    memset (list->regs_mask, 0, sizeof (list->regs_mask));
>    list->strace_data = 0;
> +
> +  xfree (list->aexpr_list);
> +  xfree (list->list);
> +}

This change is not mentioned in the ChangeLog.

> +
> +/* Wrapper to function clear_collection_list.  */

"Wrapper for".  "to" would be used to introduce a
rationale for the wrapping, like e.g.,
"wrapper to allow frobing ...".  I suggest mentioning cleanups,
like e.g.,:

/* A cleanup wrapper for clear_collection_list.  */

>  /* Reduce a collection list to string form (for gdb protocol).  */
> @@ -1606,37 +1634,50 @@ encode_actions_1 (struct command_line *action,
>      }				/* for */
>  }
>  
> -/* Render all actions into gdb protocol.  */
> -
> -void
> -encode_actions (struct bp_location *tloc, char ***tdp_actions,
> -		char ***stepping_actions)
> +static void
> +encode_actions (struct bp_location *tloc,
> +		struct collection_list *tracepoint_list,
> +		struct collection_list *stepping_list)

Missing docu.  Ah, found it in patch 4.  (would have been
best merged here).

>  {
> -  char *default_collect_line = NULL;
> -  struct command_line *actions;
> -  struct command_line *default_collect_action = NULL;
>    int frame_reg;
>    LONGEST frame_offset;
> -  struct cleanup *back_to;
> -
> -  back_to = make_cleanup (null_cleanup, NULL);
> +  struct command_line *actions;
> +  struct cleanup *old_chain;
>  
> -  clear_collection_list (&tracepoint_list);
> -  clear_collection_list (&stepping_list);
> +  old_chain = make_cleanup (null_cleanup, NULL);
>  
> -  *tdp_actions = NULL;
> -  *stepping_actions = NULL;
> +  actions = all_tracepoint_actions_and_cleanup (tloc->owner);
>  
>    gdbarch_virtual_frame_pointer (tloc->gdbarch,
>  				 tloc->address, &frame_reg, &frame_offset);
>  
> -  actions = all_tracepoint_actions_and_cleanup (tloc->owner);
> -
>    encode_actions_1 (actions, tloc, frame_reg, frame_offset,
> -		    &tracepoint_list, &stepping_list);
> +		    tracepoint_list, stepping_list);
> +
> +  do_cleanups (old_chain);
> +
> +  memrange_sortmerge (tracepoint_list);
> +  memrange_sortmerge (stepping_list);
> +}
> +
> +/* Render all actions into gdb protocol.  */
> +
> +void
> +encode_actions_rsp (struct bp_location *tloc,
> +		    char ***tdp_actions, char ***stepping_actions)
> +{
> +  struct cleanup *back_to;
> +  struct collection_list tracepoint_list, stepping_list;
> +
> +  back_to = make_cleanup (null_cleanup, NULL);
>  
> -  memrange_sortmerge (&tracepoint_list);
> -  memrange_sortmerge (&stepping_list);
> +  init_collection_list (&tracepoint_list);
> +  init_collection_list (&stepping_list);
> +
> +  make_cleanup (do_clear_collection_list, &tracepoint_list);
> +  make_cleanup (do_clear_collection_list, &stepping_list);

Thanks for splitting all this up into pieces.  Much appreciated.

However, there's a weird back and forth dance going on between this
patch and patch 4, making the reader dizzy.  :-)  Patch 4 moves
these back to encode_actions...  There's only one caller of
encode_actions in this patch, so it'd be better to leaving encode_actions
as a single function in this patch, and leave the split for one of
the follow ups (possible patch 4), explaining that the split allows
reusing the encode bits without generating the RSP, necessary for
the MI command's implementation.

> +
> +  encode_actions (tloc, &tracepoint_list, &stepping_list);

-- 
Pedro Alves