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

[Phil Muldoon <pmuldoon at redhat dot com>]

On 12/03/13 20:43, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
> Needs an intro comment.  'arg' should be const.
> 
> Phil> +  if (arg && (strcmp (arg, "--no-frame-filters") == 0))

> We recently decided to use explicit NULL checks, like 'arg != NULL'.

Ok.
 
> Phil> +  /* Parse arguments.  In this instance we are just looking for
> Phil> +     --no-frame-filters.  */
> Phil> +  while (1)
> Phil> +    {
> Phil> +      int oind = 0;
> Phil> +      char *oarg;
> Phil> +      int opt = mi_getopt ("-stack-list-frames", argc, argv,
> Phil> +			   opts, &oind, &oarg);
> 
> 
> Phil> +      /* We cannot pass -1 to frame_low, as that would signify a
> Phil> +      relative backtrace from the tail of the stack.  So, in the case
> Phil> +      of frame_low == -1, assign and increment it.  */
> Phil> +      if (py_frame_low == -1)
> Phil> +	py_frame_low++;
> 
> I didn't check, but I assume it is ok for frame_high==-1 in the call?

Yes, that indicates until end of stack.

> Phil> +  /* Run the inbuilt backtrace if there are no filters registered, or
> Phil> +     if there was an error in the Python backtracing output, or if
> Phil> +     frame-filters are disabled.  */
> Phil> +  if (! frame_filters || raw_arg || result == PY_BT_ERROR
> Phil> +      || result == PY_BT_NO_FILTERS)
> 
> It seems to me that the PY_BT_ERROR case should not be here, since the
> MI client could get a partially filtered trace followed by the normal
> trace.

Yeah you are right.  It's a pain to figure out what to do here.  I suspect in 
MI case just abandon the trace.
 
> 'result' is initialized to 0 but it seems like it should be initialized
> to some PY_BT_ enum value; and have the right type.

Oops.

> Phil> +    error (_("-stack-list-locals: Usage: [--no-frame-filters] PRINT_VALUES"));
> 
> I think -stack-list-frames takes "-no-frame-filters" but this and others
> take the "--" form.  That seems confusing.

They all take the "--" notation
 
> Phil> +       result = apply_frame_filter (frame, flags, print_value,
> Phil> +				    current_uiout, 0,0);
> 
> Missing space after the ",".
> 
> Phil> +   if (! frame_filters || raw_arg || result == PY_BT_ERROR
> Phil> +       || result == PY_BT_NO_FILTERS)
> Phil> +     {
> Phil> +       list_args_or_locals (locals, print_value, frame);
> 
> Same comments about PY_BT_ERROR and 'result'.
> Actually this applies everywhere.

Yeah everywhere in MI, I agree now.

Cheers,

Phil