RE: [PATCH v3 11/12] btrace: Remove bfun_s vector.

["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 6:27 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 11/12] btrace: Remove bfun_s vector.
> 
> On 2017-05-09 02:55, Tim Wiederhake wrote:
> > 2017-05-09  Tim Wiederhake  <tim.wiederhake@intel.com>
> >
> > gdb/ChangeLog:
> >
> > 	* btrace.c: Remove typedef bfun_s.
> > 	(ftrace_new_gap): Directly add gaps to the list of gaps.
> > 	(btrace_bridge_gaps, btrace_compute_ftrace_bts,
> pt_btrace_insn_flags,
> > 	ftrace_add_pt, btrace_compute_ftrace_pt, btrace_compute_ftrace_1,
> > 	btrace_finalize_ftrace, btrace_compute_ftrace): Use std::vector
> > 	instead of gdb VEC.
> 
> Looks good, just two nits.
> 
> > @@ -527,15 +524,15 @@ ftrace_new_gap (struct btrace_thread_info
> > *btinfo, int errcode)
> >      }
> >
> >    bfun->errcode = errcode;
> > +  gaps.push_back (bfun->number);
> >
> >    ftrace_debug (bfun, "new gap");
> >
> >    return bfun;
> >  }
> >
> > -/* Update BFUN with respect to the instruction at PC.  BTINFO is the
> > branch
> > -   trace information for the current thread.  This may create new
> > function
> > -   segments.
> > +/* Update the current function segment at the end of the trace in
> > BTINFO with
> > +   respect to the instruction at PC.  This may create new function
> > segments.
> >     Return the chronologically latest function segment, never NULL.  */
> 
> Ah here it is!  Please move it to the appropriate patch (#6 I believe).

Moved.

> > @@ -978,16 +970,15 @@ btrace_bridge_gaps (struct thread_info *tp, VEC
> > (bfun_s) **gaps)
> >      {
> >        /* Let's try to bridge as many gaps as we can.  In some cases,
> > we need to
> >  	 skip a gap and revisit it again after we closed later gaps.  */
> > -      while (!VEC_empty (bfun_s, *gaps))
> > +      while (!gaps.empty ())
> >  	{
> > -	  struct btrace_function *gap;
> > -	  unsigned int idx;
> > -
> > -	  for (idx = 0; VEC_iterate (bfun_s, *gaps, idx, gap); ++idx)
> > +	  for (const auto& number : gaps)
> 
> If you don't intend to modify number (the value in the vector), I think
> it would be actually more efficient to not use a reference to iterate on
> ints.
> 
> Also, I agree with using auto when the type it replaces is visually
> scary, but in this case I think it would be as readable and more
> informative to use the actual type:
> 
>    for (unsigned int number : gaps)

Done.

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