Re: [PATCH] Trace state variables

[Stan Shebs <stan@codesourcery.com>]

Tom Tromey wrote:
>>>>>> "Stan" == Stan Shebs <stan@codesourcery.com> writes:
>>>>>>             
>
> Stan> Since internalvar handling has changed recently, I've left off the
> Stan> value.c bit to make "print $foo" work.
>
> Will this be forthcoming?
>   
Yep, just have to figure out the new way of things.
> Stan> +     case BINOP_ASSIGN:
>
> It seems a little odd to add BINOP_ASSIGN but not BINOP_ASSIGN_MODIFY.
>   
<groan> You're going to make me do it, aren't you. :-)
> Stan>   extern int remote_supports_cond_tracepoints (void);
> Stan> + extern char *unpack_varlen_hex (char *buff, ULONGEST *result);
>
> It seems like this could be in a header somewhere.
>   
This is one of the temporary hacks pending target-vectorization of 
tracepoint ops.
> Stan> + /* The list of all trace state variables.  We don't retain pointers to
> Stan> +    any of these for any reason - API is by name or number only - so it
> Stan> +    works to have a vector of objects.  */
> Stan> + 
> Stan> + VEC(tsv_s) *tvariables;
> Stan> + 
> Stan> + /* The next integer to assign to a variable.  */
> Stan> + 
> Stan> + int next_tsv_number = 1;
>
> It seems like these, plus some of the new functions, could be static.
> It is hard to say for sure since I don't know what future patches might do.
>   
They could be, yeah, remote.c code will handle tsv's one-by-one, no need 
to pass whole list.
> Stan> + static void
> Stan> + tvariables_info (char *args, int from_tty)
> [...]
> Stan> +   printf_filtered (_("Name\t\t  Initial\tCurrent\n"));
>
> I think that brand-new formatted output should be done using the ui_out
> machinery.  Is there some drawback to doing that?  I really don't
> know... I would have expected ui_out to be used universally but instead
> it seems somewhat random, and I don't know why.  (I assume that whoever
> was doing this transition ran out of steam... ?)
>   
I can't even remember why I didn't use ui_out.  Well, that's why we do 
reviews... :-)

Stan