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

[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: 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).

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

Thanks,

Simon