Re: [PATCH] gdb: make iterate_over_threads a template function

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

On 2021-05-22 12:51 p.m., Andrew Burgess wrote:> This started off as changing the return type of the callback function
> that iterate_over_threads takes from int to bool.
> 
> But then I thought, why not make the whole thing templated, and remove
> the need to pass void* arguments around.
> 
> So now we can do:
> 
>   bool
>   callback_1 (struct thread_info *tp)
>   { ... }
> 
>   bool
>   callback_2 (struct thread_info *tp, const struct some_type &obj)
>   { ... }
> 
>   void
>   some_other_part_of_gdb ()
>   {
>     iterate_over_threads (callback_1);
> 
>     iterate_over_threads (callback_2, object_of_some_type);
>   }
> 
> Which seems much nicer to me.
> 
> While updating all the callback functions I took this opportunity to
> remove a code construct I really hate:
> 
>   if (condition)
>     return true;
>   return false;
> 
> This has now become:
> 
>   return (condition);
> 
> There should be no user visible changes after this commit.

I kind of recall that when I introduced the find_thread /
for_each_thread functions in GDBserver, I had done the way you did, but
Pedro suggested to just have a single Func parameter (like it is now),
and that we could use lambdas if needed.  And that the syntax

    for_each_thread (my_callback, some, args);

... doesn't make it super clear what's happening.  Versus:

    for_each_thread ([captures] (thread_info *thread)
      {
         my_callback (thread, some, args);
      });

... which makes it look kind of like a regular for loop.

But speaking of for loops, the most readable and straightforward of all
would probably be to just use range-for loop directly.  Instead of
writing:

  iterate_over_threads (proceed_thread_callback, pid);

write:

  for (thread_info *thread : all_threads_safe ())
    proceed_thread (thread, pid);

And if it's easy to determine that the for loop body won't delete the
thread, use all_threads instead of all_threads_safe (I wouldn't be
suprised if the cost of using all_threads_safe over all_threads was
insignificant, so perhaps we could have only all_threads, and make it
safe by default).

For the cases where iterate_over_threads is used to find a thread (i.e.
the callback doesn't always return false), then it could also be
transformed in a ranged-for plus an "if" plus a "break".  But that just
makes the calling code more complex, I don't suggest that.  I would
instead suggest to keep using iterate_over_threads, but rename it
find_thread.  find_thread speaks more about what the intent is, whereas
iterate_over_threads speaks about the mean.  So something like:

  t = find_thread ([captures] (thread_info *thread)
    {
      return my_predicate (thread);
    };

Having find_thread in GDB would align it with how it is in GDBserver.
And I think we could get rid of for_each_thread in GDBserver in favor of
range-based for loops.

Simon