[patch][python] 1 of 5 - Frame filter Python C code changes.

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> I was just trying to show a case where you have frame wrappers,
Phil> wrapping other frame wrappers, etc, until you get to BaseFrameWrapper
Phil> which wraps a gdb.Frame.  What notation should I use to illustrate
Phil> this?

Yeah, I don't know.  Maybe spell it out more.

Phil> +        if len(lvars) == 0:
Phil> +            return None
Tom> 
Tom> It is curious that this is needed.
Tom> Will whatever code eventually handles display of this do something
Tom> different for an empty list?

Phil> If this function returns None, then GDB will not print any frame
Phil> locals.  It can probably be written differently in the Python code.
Phil> But fetch_frame_locals has to return a None, or an iterator
 
Yeah, but what happens if the iterator has no elements?
It seems to me that this should work the same as the None case.
But then you don't need the special check above.

Tom> It seems strange to have both _get_sort_priority and _get_priority.
Tom> Similarly elsewhere.

Phil> This was because the Python sort/filter functions (in invoke) stop if
Phil> it encounters an error.  So if there is a badly implemented filter
Phil> (say one that does not have a "priority" attribute) then all frame
Phil> filters will not run.  This was to try and make the system more
Phil> robust.

It seems even more robust to just use the safe variant everywhere.

Phil> Sure, but I think if a class does not have an attribute which is
Phil> mandatory, we should not just set it blindly.

I think it is fine.  It is mandatory and documented as such.

Phil> +    if hasattr(filter_item, "enabled"):
Phil> +        return filter_item.enabled
Phil> +    else:
Phil> +        raise gdb.GdbError("Cannot find class attribute 'enabled'")
Tom> 
Tom> What is wrong with just fetching it and letting Python throw the
Tom> exception?

Phil> This was a case again, of trying to make the exception more relevant
Phil> to the user where a frame filter is badly implemented.  I have no
Phil> strong feelings here either.

I think the standard message is just as clear:

AttributeError: 'str' object has no attribute 'method'

Tom> If the 'invoke' function is meant to be used from outside the new
Tom> commands, it should probably not be in gdb.command.

Phil> It was easier to write "invoke" in Python than in C, and yes it is
Phil> only meant for GDB to call.  Good point on the gdb.Command issue
Phil> though.

Yeah, I don't mind it being in Python.  I think it is better that way
too.

Phil> +    DICTIONARY is the name of the frame filter dictionary on which to
Phil> +    operate.  Named dictionaries are: "global" for the global frame
Phil> +    filter dictionary, "progspace" for the program space's framefilter
Phil> +    dictionary.  If either of these two are not specified, the
Phil> +    dictionary name is assumed to be the name of the object-file name.
Tom> 
Tom> It would be nice if these commands had completion methods that did the
Tom> right thing.

Phil> Completion of the printer name?

Completion of all the arguments - the printer name but also the
dictionary.

Phil> +extract_sym (PyObject *obj, char **name, struct symbol **sym,
Phil> +	     const struct language_defn **language)
Tom> [...]
Phil> +	  *language = current_language;
Tom> 
Tom> Why current_language and not python_language?

Phil> I guess I am not clear on the difference between the two?

There may not be a difference here, but it is weird to read the Python
code and see a reference to current_language when python_language
exists.

Tom> 
Phil> +  if (ui_out_is_mi_like_p (out))
Phil> +    {
Phil> +      struct type *type;
Phil> +
Phil> +      type = check_typedef (value_type (val));
Phil> +      if (mi_print_type == PRINT_ALL_VALUES)
Phil> +	should_print = 1;
Phil> +      else if (mi_print_type == PRINT_SIMPLE_VALUES
Phil> +	       && TYPE_CODE (type) != TYPE_CODE_ARRAY
Phil> +	       && TYPE_CODE (type) != TYPE_CODE_STRUCT
Phil> +	       && TYPE_CODE (type) != TYPE_CODE_UNION)
Phil> +	should_print = 1;
Phil> +    }
Tom> 
Tom> Rather than making this all conditional, you could make the API simpler
Tom> to understand by having the CLI always pass PRINT_ALL_VALUES.

Phil> I am not sure what you mean here with CLI, as this is an MI case?
 
Sorry, what I mean is, don't check ui_out_is_mi_like_p at all, just
always respect the "print type", and have the CLI code pass in the
appropriate value to get the behavior it wants.

Phil> +		     int mi_print_type,
Phil> +		     int print_mi_args_flag,
Phil> +		     const char *cli_print_frame_args_type,
Tom> 
Tom> cli_print_frame_args_type isn't documented in the function comment.
Tom> The comment refers to print_mi_args, not print_mi_args_flag.
 
Tom> This seems like a lot of duplication to me.
Tom> Why not use a single enum for all cases?  That would be a lot simpler.
Tom> It is fine to introduce a new enum just for this code.

Phil> Because CLI and MI use those enums internally, and each print values
Phil> differently depending on type.  So I get those values from MI/CLI.
Phil> They provide them, I did not create them.  But I still have to account
Phil> for their instructions to match existing CLI/MI expectations of output.

I think it is better to present a simpler API to the callers.
It is fine to introduce new enums and to make the CLI and MI code
convert their requests.  Or, better, unify the enums across the entire
code base.

Phil> +  module = PyImport_ImportModule ("gdb.command.frame_filters");
Tom> 
Tom> Yeah, definitely should be a different module.
Tom> 
Tom> I think 'invoke' should be renamed, too; and documented.
Tom> It is nice for things like this if there is an obvious way from Python
Tom> to accomplish the same thing that gdb does internally.  We didn't think
Tom> of this for value pretty printers (though you can still do it), but I'd
Tom> like us to consider it for new additions.

Phil> It was very much simpler to write "invoke" in Python, than in C.  In
Phil> fact where I could write something in Python, I did.  Most of the C
Phil> code is just printing the frames.

Yes, my comment isn't about C, but rather which module the function
lives in.

I think something like 'gdb.frames' would be better, analogous to
gdb.types.

And, rename the function and document it so that user code can call it
to easily iterate over filtered frames.

Tom> You're creating cleanups for everything except this.
Tom> It doesn't really matter, since this code is not exposed to gdb
Tom> exceptions, but I wonder why.  Simpler to make it all uniform.

Phil> This was a late addition to track frames printed and is simply a
Phil> blimp.  I think I did not create a cleanup for this as it will always
Phil> fall through to the free.

The question is just about uniformity.
All code in that function will fall through.
So I think it should either do explicit frees everywhere, or use
cleanups everywhere.

Tom