Re: [patch] info threads sort by name and name regex matching

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Wednesday, August 22 2012, Aaron Gamble wrote:

> On Wed, Aug 22, 2012 at 11:51 AM, Tom Tromey <tromey@redhat.com> wrote:

>> Aaron> With this patch sorting by name will happen always. (Perhaps a switch
>> Aaron> to enable sorting would be better?)
>>
>> Yes, I think so.  I think sorting by name makes sense but it isn't
>> perhaps always what you want.  Sorting by number has the nice feature
>> that it is stable.
>>
>> Aaron> Regex matching is specified by doing 'info threads r<regex>'. This is
>> Aaron> not ambiguous with previous behavior where parameters to info threads
>> Aaron> were only numbers. (spaces between 'r' and <regex> are ignored)
>>
>> I'm curious why you chose this particular spelling.
>> Other possible approaches would be a subcommand, or a flag like "-r".
>> Either of these is perhaps more in keeping with gdb tradition.
>>
>> I'm interested in other opinions here too.
>>
>> Expect some bikeshedding on this point.
> I picked a single letter for it to be quick and easy to type. I
> suppose it's possible 'info threads' could be expanded to do other
> sorts of matching where different flags would be useful.
>
> How about 'info threads [ -a ] [ id.. | -n <name regex> ]'
>
> -a - Sort alphabetically by name
> -n regex - Match thread names with regex
>
> Of course then we could do sorting based on the name of the function a
> thread is in or other sorts of sorting. So other sorting flags would
> need to be introduced.

Why not `-r regex'?  I think it would be more clear.

Other commands (`sharedlibrary', `info variables', etc) take a regex as
their first argument, without requiring a modifier like `-r'.  If the
"pipe" patch were already in, this alphabetical sorting would not be
needed...  Anyway, just thinking here.

> *snip*
>> Aaron> +      threads = xmalloc (sizeof (*threads) * thread_list_size);
>> Aaron> +      make_cleanup (free, threads);
>>
>> I think making a VEC here would be better.
>> Then you wouldn't need thread_list_size at all.
>
> Hmm, not sure if this would improve performance at all. A one time
> allocation bounded by the number of threads vs VEC's implementation of
> expanding arrays. I'll wait for others feedback.

I guess Tom did not suggest this because of performance per se, but
rather because if you have to make a list in GDB then it is already a
convention to use VEC for these things.

BTW, I noticed one minor nit in your patch: you do not obey the "TAB
vs. space" rule.  Basically, if you have 8 spaces, then you should
convert it to a TAB.  Emacs does this for free, but if you're using Vim
you can ask me offlist and I can send you a little function that does
that.

Thanks,

-- 
Sergio