Re: [patch 1/8] Download tracepoint on location level

[Pedro Alves <pedro@codesourcery.com>]

On Wednesday 09 November 2011 03:34:01, Yao Qi wrote:

> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -674,7 +674,7 @@ update_current_target (void)
>        INHERIT (to_supports_enable_disable_tracepoint, t);
>        INHERIT (to_supports_string_tracing, t);
>        INHERIT (to_trace_init, t);
> -      INHERIT (to_download_tracepoint, t);
> +      INHERIT (to_download_tracepoint_loc, t);

Please drop the _loc from the function name.  We don't have it
in the other methods like to_enable_tracepoint/to_disable_tracepoint
which take a location too, and others like to_insert_breakpoint
aren't called to_insert_breakpoint_gdbarch_target_info either.

> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -9835,9 +9835,9 @@ remote_download_command_source (int num, ULONGEST addr,
>  }
>  
>  static void
> -remote_download_tracepoint (struct breakpoint *b)
> +remote_download_tracepoint_loc (struct bp_location *loc)
>  {

Ditto.

> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -686,8 +686,10 @@ struct target_ops
>      /* Prepare the target for a tracing run.  */
>      void (*to_trace_init) (void);
>  
> -    /* Send full details of a tracepoint to the target.  */
> -    void (*to_download_tracepoint) (struct breakpoint *t);
> +    /* Send full details of a tracepoint location to the target.  Target may
> +       install or not install tracepoint locations to inferior, which is
> +       determined by target's factors, such tracing state.  */

This comment doesn't belong in this patch.  And I think it only
made sense on a previous version of the patch series.

> +    void (*to_download_tracepoint_loc) (struct bp_location *location);
>  
>      /* Send full details of a trace state variable to the target.  */
>      void (*to_download_trace_state_variable) (struct trace_state_variable *tsv);
> @@ -1474,8 +1476,8 @@ extern int target_search_memory (CORE_ADDR start_addr,
>  #define target_trace_init() \
>    (*current_target.to_trace_init) ()
>  
> -#define target_download_tracepoint(t) \
> -  (*current_target.to_download_tracepoint) (t)
> +#define target_download_tracepoint_loc(t) \
> +  (*current_target.to_download_tracepoint_loc) (t)

Drop _loc.

Okay with those changes.

-- 
Pedro Alves