Re: [PATCH v3 06/12] btrace: Remove constant arguments.

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-05-09 02:55, Tim Wiederhake wrote:
> 2017-05-09  Tim Wiederhake  <tim.wiederhake@intel.com>
> 
> gdb/ChangeLog:
> 
> 	* btrace.c (ftrace_new_function, ftrace_new_call, ftrace_new_tailcall,
> 	ftrace_new_return, ftrace_new_switch, ftrace_new_gap,
> 	ftrace_update_function): Remove arguments that implicitly were always
> 	BTINFO->END.
> 	(btrace_compute_ftrace_bts, ftrace_add_pt, btrace_compute_ftrace_pt):
> 	Don't pass BTINFO->END.

Looks good, just a few comments below.

> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index cb30dcf..1bd11f0 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -202,19 +202,19 @@ ftrace_function_switched (const struct
> btrace_function *bfun,
>    return 0;
>  }
> 
> -/* Allocate and initialize a new branch trace function segment.
> +/* Allocate and initialize a new branch trace function segment at the 
> end of
> +   the trace.
>     BTINFO is the branch trace information for the current thread.
> -   PREV is the chronologically preceding function segment.
>     MFUN and FUN are the symbol information we have for this function.  
> */
> 
>  static struct btrace_function *
>  ftrace_new_function (struct btrace_thread_info *btinfo,
> -		     struct btrace_function *prev,
>  		     struct minimal_symbol *mfun,
>  		     struct symbol *fun)
>  {
> -  struct btrace_function *bfun;
> +  struct btrace_function *bfun, *prev;
> 
> +  prev = btinfo->end;

Note that we can now declare variables at the point we use it, and drop 
the struct keyword, like:

   btrace_function *prev = btinfo->end;

It's up to you, you can continue with your current style if you wish.

>    bfun = XCNEW (struct btrace_function);
> 
>    bfun->msym = mfun;
> @@ -238,7 +238,7 @@ ftrace_new_function (struct btrace_thread_info 
> *btinfo,
>      }
> 
>    btinfo->functions.push_back (bfun);
> -  return bfun;
> +  return btinfo->end = bfun;

Err I'm really not a fan of assignment as a side effect.

> @@ -515,13 +510,13 @@ ftrace_new_gap (struct btrace_thread_info 
> *btinfo,
>     Return the chronologically latest function segment, never NULL.  */
> 
>  static struct btrace_function *
> -ftrace_update_function (struct btrace_thread_info *btinfo,
> -			struct btrace_function *bfun, CORE_ADDR pc)
> +ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR 
> pc)

The comment of this function would need to be updated as well.

Thanks,

Simon