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

["Wiederhake, Tim" <tim.wiederhake@intel.com>]

Hi Simon,

Thanks for reviewing!

> -----Original Message-----
> From: Simon Marchi [mailto:simon.marchi@polymtl.ca]
> Sent: Wednesday, May 10, 2017 4:46 AM
> To: Wiederhake, Tim <tim.wiederhake@intel.com>
> Cc: gdb-patches@sourceware.org; Metzger, Markus T
> <markus.t.metzger@intel.com>
> Subject: Re: [PATCH v3 06/12] btrace: Remove constant arguments.
> 
> 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.

I do prefer declaring (and if at all possible, initialising) variables at the point of first usage, too. For now I'll stick with the current style to leave this kind of change to a separate C++-ification patch set.

> >    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.

This line is removed by the very next patch in the series. Anyway, I changed it.

> 
> > @@ -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.

Fixed.

> Thanks,
> 
> Simon

Regards,
Tim
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928