Re: [patch][python] 2 of 5 - Frame filter MI code changes.

[Phil Muldoon <pmuldoon at redhat dot com>]

On 12/05/2012 05:11 PM, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

> Phil> +/* True if we want to allow Python-based frame filters.  */
> Phil> +static int frame_filters = 0;
> Phil> +
> Phil> +void
> Phil> +stack_enable_frame_filters (void)
> Phil> +{
> Phil> +  frame_filters = 1;
> Phil> +}
> 
> I don't think you need this function, see below.
> 
> Phil> +static int
> Phil> +parse_no_frames_option (char *arg)
> Phil> +{
> Phil> +  if (arg && (strcmp (arg, "--no-frame-filters") == 0))
> Phil> +    return 1;
> Phil> +
> Phil> +  return 0;
> 
> I'd prefer it if the various callers were changed to use mi_getopt.
> This provides uniformity and lets us add options later.

If there was uniformity then I would agree, but as far as I looked
there wasn't.  Some MI commands use mi_getopt, some parse their own
options, some allow long options ("--"), others do not, and mi_getopt
does not handle long options in any case (and huge amounts of other
useful getopt functions too).  I wrote a patch for mi_getopts to
handle long options, but why do we even need another implementation of
getopt like functionality?

So I decided to just leave be, and parse options as each command has
previously done so.  Maybe I should have written a cleanup patch
before hand.

I wanted to mention something else about MI.  I recently discovered in
the GDB manual that -stack-list-locals, -stack-list-arguments are
considered depreciated.  Not even sure if we should add frame filter
logic to them.  What do you think?

> Phil> +  if (! raw_arg && frame_filters)
> Phil> +    {
> Phil> +      int count = frame_high;
> Phil> +      int flags = PRINT_LEVEL | PRINT_FRAME_INFO;
> Phil> +
> Phil> +      if (frame_high != -1)
> Phil> +	count = (frame_high - frame_low) + 1;
> Phil> +
> Phil> +      result = apply_frame_filter (fi, flags, 0, NULL, current_uiout,
> Phil> +				   count);
> 
> I don't think I follow the high/low logic here.
> 
> How does this code strip off the first 'frame_low' frames?

fi is unwound to the position of frame_low in a loop preceding this
call.  This is existing code, and not in the patch context.  It is as
follows:

  /* Let's position fi on the frame at which to start the
     display. Could be the innermost frame if the whole stack needs
     displaying, or if frame_low is 0.  */
  for (i = 0, fi = get_current_frame ();
       fi && i < frame_low;
       i++, fi = get_prev_frame (fi));


> 
> Also, Do frame_low and frame_high refer to "raw" or "cooked" frames?
> I tend to think they should refer to cooked ones, but I think at least
> the answer should be explicit and documented.

In the existing mi sense, they just refer to frames on the stack.  I
followed this logic, but something I am still unsure of is if a frame
is elided between frame low, and frame high, if that should be
counted.  I think it should.

> Phil>  void
> Phil> +mi_cmd_enable_frame_filters (char *command, char **argv, int argc)
> Phil> +{
> Phil> +  if (argc != 0)
> Phil> +    error (_("-enable-frame-filters: no arguments allowed"));
> Phil> +
> Phil> +  stack_enable_frame_filters ();
> 
> I think just put this into mi-cmd-stack.c and remove
> stack_enable_frame_filters.

I was curious about this, I just followed how pretty printing is done.
I have no objection though.
 
Cheers,

Phil