Re: [PATCH] Trace state variables

[Tom Tromey <tromey@redhat.com>]

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

Stan> +     case BINOP_ASSIGN:

It seems a little odd to add BINOP_ASSIGN but not BINOP_ASSIGN_MODIFY.

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.

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.

Stan> + void
Stan> + trace_variable_command (char *args, int from_tty)
Stan> + {
[...]
Stan> +   /* All the possible valid arguments are expressions.  */
Stan> +   expr = parse_expression (args);
Stan> +   old_chain = make_cleanup (free_current_contents, &expr);

I think set_cmd_completer (..., expression_completer) is probably in
order, in the file's initialize function.

Stan> + void
Stan> + delete_trace_variable_command (char *args, int from_tty)

It would be nice to have a completer function for this.

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... ?)

Stan> + typedef struct trace_state_variable tsv_s;
Stan> + DEF_VEC_O(tsv_s);

AFAICT, this VEC is only used in a single .c file.  It is better to put
the DEF_VEC there, to avoid duplicating the static functions in other
compilation units where they aren't needed.

Tom