[patch][python] 1 of 5 - Frame filter Python C code changes.
Tom Tromey
tromey@redhat.com
Wed Dec 5 17:03:00 GMT 2012
>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
Hi. Lots of notes on this one.
Phil> diff --git a/gdb/python/lib/gdb/BaseFrameWrapper.py b/gdb/python/lib/gdb/BaseFrameWrapper.py
Phil> + If the result of frame filters running means we have one gdb.Frame
Phil> + wrapped by multiple frame wrappers, all sub-classed from
Phil> + BaseFrameWrapper:
Phil> +
Phil> + Wrapper1(Wrapper2(BaseFrameWrapper(gdb.Frame)))
I think this notation is a bit weird.
Phil> + @staticmethod
Phil> + def is_limited_frame(frame):
Probably should start with an underscore.
Phil> + def __init__(self, base):
Phil> + super(BaseFrameWrapper, self).__init__(base)
Phil> + self.base = base
Perhaps a leading underscore for 'base'.
However -- the superclass sets 'self.frame'. If that is considered API,
then you don't need 'base' at all. And if it isn't considered API, it
should be renamed.
Phil> +class BaseSymValueWrapper():
Probably should be BaseSymValueWrapper(object) for Python 3.
Same for FrameVars.
Phil> + """A container class conforming to the Symbol/Value interface
Phil> + which holds frame locals or frame arguments."""
Phil> + def __init__(self, symbol, value):
Phil> + self.sym = symbol
Phil> + self.val = value
As far as I can tell this is always instantiated with value == None.
That seems wrong.
Phil> + def fetch_frame_locals(self):
Phil> + """Public utility method to fetch frame local variables for
Phil> + the stored frame. Frame arguments are not fetched. If there
Phil> + are not frame local variables, return None."""
Typo: should be "there are no".
[ fetch_frame_locals ]
Phil> + if len(lvars) == 0:
Phil> + return None
It is curious that this is needed.
Will whatever code eventually handles display of this do something
different for an empty list?
Phil> + def fetch_frame_args(self):
Phil> + """Public utility method to fetch frame argument for the
Phil> + stored frame. Frame arguments are the only type fetched. If
Phil> + there are no frame arguments variables, return None."""
I think "frame argument" - not plural.
Phil> + def get_value(self, sym, block):
Phil> + """Public utility method to fetch a value from a symbol."""
What calls this?
Phil> +class FrameIterator(object):
[...]
Phil> + def next (self):
Phil> + """__next__ implementation.
I think just "next", not "__next__".
Phil> + self.frame = result.older ()
No space before parens in Python.
It is hard to remember.
Phil> +class FrameWrapper (object):
Phil> + """Interface for a Frame Wrapper."""
If this is just for documentation, how about we just put it all in the
docs and delete the class?
Then we can rename "BaseFrameWrapper". I find the BaseFrameWrapper /
FrameWrapper distinction a bit confusing.
Narrowly speaking, this class is misnamed anyhow, since it doesn't
actually wrap anything.
Phil> +def _parse_arg(cmd_name, arg):
Phil> + """ Internal worker function to take an argument and return a
Phil> + tuple of arguments.
...
Phil> + object_list = argv[0]
Phil> + argument = argv[1]
Phil> +
Phil> + return(object_list, argument)
It seems like you can just return argv here.
Phil> +def _get_priority(filter_item):
Phil> + """ Internal worker function to return the frame-filter's priority.
It seems strange to have both _get_sort_priority and _get_priority.
Similarly elsewhere.
Phil> +def _set_priority(filter_item, priority):
Phil> + """ Internal worker function to set the frame-filter's priority.
Phil> +
Phil> + Arguments:
Phil> + filter_item: An object conforming to the frame filter
Phil> + interface.
Phil> + priority: The priority to assign as an integer.
Phil> +
Phil> + Raises:
Phil> + gdb.GdbError: When the priority attribute has not been
Phil> + implemented.
Phil> + """
Phil> +
Phil> + if hasattr(filter_item, "priority"):
Phil> + filter_item.priority = priority
Phil> + else:
Phil> + raise gdb.GdbError("Cannot find class attribute 'priority'")
Why check for priority before setting it? It seems simpler to just set
it. Similarly elsewhere.
Phil> + if hasattr(filter_item, "enabled"):
Phil> + return filter_item.enabled
Phil> + else:
Phil> + raise gdb.GdbError("Cannot find class attribute 'enabled'")
What is wrong with just fetching it and letting Python throw the
exception?
Phil> +def _return_list(name):
Phil> + """ Internal Worker function to return the frame filter
Phil> + dictionary, depending on the name supplied as an argument. If the
Phil> + name is not "global" or "progspace", it is assumed to name an
Phil> + object-file.
Phil> +
Phil> + Arguments:
Phil> + name: The name of the list, as specified by GDB user commands.
Phil> +
Phil> + Returns:
Phil> + A dictionary object.
Phil> +
Phil> + Raises:
Phil> + gdb.GdbError: A dictionary of that name cannot be found.
Phil> + """
Phil> +
Phil> + if name == "global":
Extra space.
Phil> + sorted_frame_filters = copy.copy(all_filters)
Phil> + sorted_frame_filters.sort(key = _get_sort_priority,
Phil> + reverse = True)
I think for Python 3 we recently switched to using 'sorted' instead of
the in-place sort.
Hmm, that patch doesn't seem to have gone in.
Phil> + for ff in sorted_list:
Phil> + frame_iterator = ff[1].filter(frame_iterator)
I don't understand the "[1]" here.
If the 'invoke' function is meant to be used from outside the new
commands, it should probably not be in gdb.command.
Phil> +# GDB Commands.
Phil> +class SetFilterPrefixCmd(gdb.Command):
I think this probably needs some help text.
Phil> +class ShowFilterPrefixCmd(gdb.Command):
Likewise.
Phil> +class InfoFrameFilter(gdb.Command):
Phil> + """GDB command to list all registered frame-filters.
Starting the text with "GDB command" is redundant.
Phil> + def __init__(self):
This should be the first method.
Phil> + def print_list(self, title, filter_list):
Phil> + """Print a list."""
Just remove this text.
It isn't useful.
Phil> + def invoke(self, arg, from_tty):
Phil> + """GDB calls this to perform the command."""
Likewise.
Phil> +# Internal enable/disable functions.
Phil> +
Phil> +def do_enable_frame_filter_1(frame_filters, name, flag):
This just has a single caller.
I don't think it needs to be separate.
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.
It would be nice if these commands had completion methods that did the
right thing.
Phil> + object_list = argv[0]
Phil> + name = argv[1]
Phil> + priority = argv[2]
Phil> + return(object_list, name, priority)
I think you can just return argv here.
Phil> + def _set_filter_priority_1(self, frame_filters, name, priority):
I don't think this function is needed.
Phil> + object_list = argv[0]
Phil> + name = argv[1]
Phil> + return (object_list, name)
Just return argv.
I probably missed some instances of this pattern.
Phil> +def register_frame_filter_commands():
Phil> + """Call from a top level script to install the frame-filter commands."""
Phil> + InfoFrameFilter()
Phil> + SetFilterPrefixCmd()
Phil> + ShowFilterPrefixCmd()
Phil> + EnableFrameFilter()
Phil> + DisableFrameFilter()
Phil> + SetFrameFilterPriority()
Phil> + ShowFrameFilterPriority()
Phil> +
Phil> +register_frame_filter_commands()
No need to create a function just to call it once.
Phil> + OBJ is the Python object to extract the values from. **NAME is a
s/**NAME/NAME
Similarly elsewhere.
Phil> + pass-through argument where the name of the symbol will be written.
Phil> + **NAME is allocated in this function, but the caller is responsible
You probably mean *NAME here.
**NAME is a single character.
Phil> +static int
Phil> +extract_sym (PyObject *obj, char **name, struct symbol **sym,
Phil> + const struct language_defn **language)
[...]
Phil> + *language = current_language;
Why current_language and not python_language?
Phil> + PyErr_SetString (PyExc_RuntimeError,
Phil> + _("Unexpected value. Expecting a " \
Don't need a backslash here.
Phil> + PyErr_SetString (PyExc_RuntimeError,
Phil> + _("Mandatory function 'symbol' not " \
Phil> + "implemented."));
On the whole it seems simpler to just try to call the method and let
Python handle the failure, since you have to handle that case anyway.
Phil> +static int
Phil> +mi_should_print (struct symbol *sym, const char *type)
This is only ever called with constant strings for the second argument.
It is better to use an enum in this situation.
Phil> +static int
Phil> +py_print_type (struct ui_out *out, struct value *val)
[...]
Phil> + if (except.reason > 0)
Phil> + {
Phil> + PyErr_SetString (PyExc_RuntimeError,
Phil> + except.message);
Use gdbpy_convert_exception here.
This should be fixed at each TRY_CATCH.
Phil> +/* Helper function which outputs a value name to value field in a
This reads strangely.
Phil> + stream. OUT is the ui-out structure the value will be output too,
s/too/to/
Phil> +static int
Phil> +py_print_value (struct ui_out *out, struct value *val,
Phil> + struct value_print_options opts,
Phil> + int mi_print_type,
Why is mi_print_type 'int' and not enum print_values?
Phil> + /* MI disallows different value types against different options the
Phil> + client passes, so test type against option. For CLI print all
Phil> + values. */
This comment reads strangely.
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> + }
Rather than making this all conditional, you could make the API simpler
to understand by having the CLI always pass PRINT_ALL_VALUES.
Phil> +/* Helper function to call a Python method and extract an iterator
Phil> + from the result, error checking for Python exception and returns
Phil> + that are not iterators. FILTER is the Python object to call, and
Phil> + FUNC is the name of the method. Returns a PyObject, or NULL on
Phil> + error with the appropriate exception set. This function can return
Phil> + an iterator, or None. */
Phil> +
Phil> +static PyObject *
Phil> +get_py_iter_from_func (PyObject *filter, char *func)
Why not const char *?
Phil> + if (! PyIter_Check (result))
Phil> + {
Phil> + PyErr_Format (PyExc_RuntimeError,
Phil> + _(" %s function must " \
Phil> + "return an iterator."), func);
Phil> + Py_DECREF (result);
Phil> + return NULL;
Phil> + }
Phil> + else
Phil> + {
Phil> + PyObject *iterator = PyObject_GetIter (result);
I think it doesn't really make sense to both do PyIter_Check and then
call PyObject_GetIter.
Generally these patches specify the API to work on iterators. But it
seems to me that it is more flexible to work on iterables instead.
That way code could return an iterator, or an array or tuple, depending
on what is convenient to that code -- more Pythonic IMO.
This would require fixing up the docs in various places.
In this spot at least it would mean not calling PyIter_Check.
Phil> + if (! iterator)
Phil> + return NULL;
Phil> + else
Phil> + return iterator;
This can be just 'return iterator'
Phil> +/* Helper function to output a single frame argument and value to an
Phil> + output stream. This function will account for entry values if the
Phil> + FV parameter is populated, the frame argument has entry values
Phil> + associated with them, and the appropriate "set entry-value"
Phil> + options are set. Will output in CLI or MI like format depending
Phil> + on the type of output stream detected. OUT is the output stream,
Phil> + SYM_NAME is the name of the symbol. If SYM_NAME is populated then
Phil> + it must have an accompanying value in the parameter FV. FA is a
Phil> + frame argument structure. If FA is populated, both SYM_NAME and
Phil> + FV are ignored. OPTS contains the value printing options,
Phil> + MI_PRINT_TYPE is an enumerator to the value types that will be
Phil> + printed if the output is MI. PRINT_MI_ARGS indicates whether to
Phil> + output the ARGS="1" field in MI output. */
Phil> +static int
Phil> +py_print_single_arg (struct ui_out *out,
Missing blank line.
Phil> + char *sym_name,
Why not const char *?
Phil> + struct value_print_options opts,
Why by-value and not 'const struct value_print_options *opts'?
I missed it earlier but the same applies to py_print_value.
Phil> + int mi_print_type,
Phil> + int print_mi_args_flag,
Phil> + const char *cli_print_frame_args_type,
cli_print_frame_args_type isn't documented in the function comment.
The comment refers to print_mi_args, not print_mi_args_flag.
This seems like a lot of duplication to me.
Why not use a single enum for all cases? That would be a lot simpler.
It is fine to introduce a new enum just for this code.
This applies to many of the functions in this file.
Also these should have the proper enum type and not be ints.
Phil> + struct cleanup *inner_cleanup =
Phil> + make_cleanup (null_cleanup, NULL);
Phil> +
Phil> + if (fa)
Phil> + {
Phil> + language = language_def (SYMBOL_LANGUAGE (fa->sym));
Phil> + val = fa->val;
Phil> + }
Phil> + else
Phil> + val = fv;
Phil> +
Phil> + /* MI has varying rules for tuples, but generally if there is only
Phil> + one element in each item in the list, do not start a tuple. */
Phil> + if (print_mi_args_flag || mi_print_type != PRINT_NO_VALUES)
Phil> + {
Phil> + inner_cleanup =
Phil> + make_cleanup_ui_out_tuple_begin_end (out,
Phil> + NULL);
It isn't good to assign to the same cleanup variable like this.
You can just ignore the return value from this call though.
I think the name 'inner_cleanup' is odd here, considering that there is
no outer scope.
Phil> + /* If frame argument is populated, check for entry-values and the
Phil> + entry value options. */
Phil> + if (fa)
Phil> + {
Phil> + struct ui_file *stb;
Phil> +
Phil> + stb = mem_fileopen ();
Phil> +
Phil> + fprintf_symbol_filtered (stb, SYMBOL_PRINT_NAME (fa->sym),
Phil> + SYMBOL_LANGUAGE (fa->sym),
Phil> + DMGL_PARAMS | DMGL_ANSI);
Phil> + if (fa->entry_kind == print_entry_values_compact)
Phil> + {
Phil> + fputs_filtered ("=", stb);
Phil> +
Phil> + fprintf_symbol_filtered (stb, SYMBOL_PRINT_NAME (fa->sym),
Phil> + SYMBOL_LANGUAGE (fa->sym),
Phil> + DMGL_PARAMS | DMGL_ANSI);
Phil> + }
Phil> + if (fa->entry_kind == print_entry_values_only
Phil> + || fa->entry_kind == print_entry_values_compact)
Phil> + {
Phil> + fputs_filtered ("@entry", stb);
Phil> + }
Phil> + ui_out_field_stream (out, "name", stb);
Phil> + ui_file_delete (stb);
Phil> + }
Phil> + else
Phil> + /* Otherwise, just output the name. */
Phil> + ui_out_field_string (out, "name", sym_name);
If similar code appears elsewhere in gdb, how about refactoring so it
can be shared?
This seems a bit questionable given that it calls mem_fileopen but never
makes a cleanup for it.
Phil> + /* For MI print the type. */
Phil> + if (ui_out_is_mi_like_p (out)
Phil> + && mi_print_type == PRINT_SIMPLE_VALUES)
It seems weird to only do this for PRINT_SIMPLE_VALUES.
Phil> + else
Phil> + if (PyErr_Occurred ())
Merge into one line.
Phil> +static int
Phil> +enumerate_locals (PyObject *iter,
Phil> + struct ui_out *out,
Phil> + int mi_print_type,
Phil> + int indent,
Phil> + int print_mi_args_flag,
Phil> + struct frame_info *frame)
This seems to share a lot of code with enumerate_args.
Can't they be merged?
These functions call a bunch of ui_out functions outside of TRY_CATCH.
I think that isn't ok -- CLI backtraces are filtered, meaning that it is
possible for the user to type "q" at the page break, and this results in
a gdb exception being thrown.
Phil> + make_cleanup_ui_out_list_begin_end (out, "locals");
Phil> + if (locals_iter != Py_None)
Phil> + if (! enumerate_locals (locals_iter, out, mi_print_type,
Phil> + indent, 0, frame))
Phil> + goto locals_error;
In cases like this, does it really make sense to emit the "locals" field
even when it is empty? Is this what MI ordinarily does?
Phil> +static hashval_t
Phil> +hash_printed_frame_entry (const void *data)
Phil> +{
Phil> + const struct frame_info *frame = data;
Phil> +
Phil> + return htab_hash_pointer (frame);
Phil> +}
Phil> +
Phil> +/* Equality function for the printed hash. */
Phil> +
Phil> +static int
Phil> +eq_printed_frame_entry (const void *a, const void *b)
Phil> +{
Phil> + const struct frame_info *ea = a;
Phil> + const struct frame_info *eb = b;
Phil> +
Phil> + return ea == eb;
You don't need these, you can use htab_hash_pointer and htab_eq_pointer
instead.
Phil> + gdbarch = get_frame_arch (frame);
In general you can't call a gdb function outside of TRY_CATCH.
I think it is reasonably dangerous to deviate from this rule.
Phil> + Py_DECREF (result);
Phil> + }
Phil> + else
Phil> + {
Phil> + PyErr_SetString (PyExc_RuntimeError,
Phil> + _("'inferior_frame' API must be implemented."));
Phil> + goto error;
Phil> + }
This is another situation where you should just unconditionally call the
method. You only need to check for the method if the code should adapt
to its absence; if it is required, just call it and let Python report a
failure if it is not there.
Phil> + level = frame_relative_level (frame);
Unprotected call.
Phil> + char buffer[10];
Phil> + sprintf (buffer, "%d", level);
Phil> + ui_out_spaces (out, strlen (buffer) + 2);
Blank line between declarations and code.
Also we're trying to avoid sprintf now; use xnsprintf.
Phil> + ui_out_field_fmt_int (out, 2, ui_left, "level",
Phil> + level);
I wonder though if the above should use ui_out_field_skip instead of
ui_out_spaces. Or perhaps both if just the former doesn't result in the
correct output.
Phil> + func = xstrdup (dup);
Phil> + annotate_frame_function_name ();
Phil> + ui_out_field_string (out, "func", func);
Phil> + xfree (func);
Phil> +
Phil> + }
Extra blank line.
Also I think the name 'dup' here is misnomer.
It isn't actually a dup of anything.
And, there doesn't seem to be a reason to duplicate 'dup' just to print
it and then free it. Why not just print it directly?
This occurs elsewhere as well.
Phil> + /* For MI we need to deal with the "children" list population of
Phil> + elided frames, so if MI output detected do not send newline. */
Phil> + if (! ui_out_is_mi_like_p (out))
Phil> + {
Phil> + annotate_frame_end ();
Phil> + ui_out_text (out, "\n");
ui_out_text is a no-op for MI. It is usually cleaner to simply always
call it. It doesn't need a comment, either.
I think the same applies to annotations.
Occurs in other spots too.
Phil> + int success = py_print_frame (item, print_level,
Phil> + print_frame_info,
Phil> + print_args,
Phil> + print_locals,
Phil> + mi_print_type,
Phil> + cli_print_frame_args_type,
Phil> + out, indent,
Phil> + levels_printed);
Phil> +
Phil> + if (success == 0 && PyErr_Occurred ())
Phil> + {
Phil> + Py_DECREF (item);
Phil> + goto error;
Phil> + }
Can you have success == 0 but no error?
Why not return 1 in that case?
The py_print_frame doc comment doesn't explain its return values.
Oh, I see it returns a PY_BT_ constant. But then a check against 0 is
confusing, it should check against the constants.
Phil> +/* Helper function to initiate frame filter invocation at starting
Phil> + frame FRAME. */
Phil> +static PyObject *
Missing blank line.
Phil> +bootstrap_python_frame_filters (struct frame_info *frame)
Phil> +{
Phil> +
Phil> + PyObject *module, *sort_func, *iterable, *frame_obj;
Extra blank line.
Phil> + module = PyImport_ImportModule ("gdb.command.frame_filters");
Yeah, definitely should be a different module.
I think 'invoke' should be renamed, too; and documented.
It is nice for things like this if there is an obvious way from Python
to accomplish the same thing that gdb does internally. We didn't think
of this for value pretty printers (though you can still do it), but I'd
like us to consider it for new additions.
Phil> +/* Public and dispatch function for frame filters. This is the only
Not sure about that first sentence.
Phil> + publicly exported function in this file. FRAME is the source
Phil> + frame to start frame-filter invocation. FLAGS is an integer
Phil> + holding the flags for printing. The following elements of the
Phil> + FRAME_FILTER_FLAGS enum denotes makeup of FLAGS: PRINT_LEVEL is a
Phil> + flag indicating whether to print the frame's relative level in the
Phil> + output. PRINT_FRAME_INFO is a flag that indicates whether this
Phil> + function should print the frame information, PRINT_ARGS is a flag
Phil> + that indicates whether to print frame arguments, and PRINT_LOCALS,
Phil> + likewise, with frame local variables. MI_PRINT_ARGS_TYPE is an
Phil> + enum from MI that indicates which values types to print. This
Phil> + parameter is ignored if the output is detected to be CLI.
Phil> + CLI_PRINT_FRAME_ARGS_TYPE likewise is a an element of what value
Phil> + types to print from CLI. OUT is the output stream to print, and
Phil> + COUNT is a delimiter (required for MI slices). */
I think COUNT is just a count.
Phil> + int success = 0;
I think this is probably initialized improperly.
Phil> + /* If iterable is None, then there are not any frame filters
Extra space before "If". This throws off the indentation of the rest of
the comment.
Phil> + make_cleanup_py_decref (iterable);
Phil> + if (iterable == Py_None)
Phil> + {
Phil> + do_cleanups (cleanups);
Phil> + return PY_BT_NO_FILTERS;
Better I think to set 'success' and goto done.
Phil> + /* Is it an iterator */
End with a period & two spaces.
Phil> + if PyIter_Check (iterable)
I think it really should be an iterable, not an iterator.
So, remove the 'if' and call PyObject_GetIter unconditionally.
Phil> + levels_printed = htab_create (20,
Phil> + hash_printed_frame_entry,
Phil> + eq_printed_frame_entry,
Phil> + NULL);
You're creating cleanups for everything except this.
It doesn't really matter, since this code is not exposed to gdb
exceptions, but I wonder why. Simpler to make it all uniform.
Phil> + while ((item = PyIter_Next (iterator)) && count--)
If 'count-- == 0' then you will leak 'item'.
Phil> + success = py_print_frame (item, print_level,
Extra space after the '='.
Phil> + /* Do not exit on error printing the frame, continue with
Phil> + other frames. */
Phil> + if (success == PY_BT_ERROR && PyErr_Occurred ())
Checking PyErr_Occurred makes me think that the return results could use
a cleanup. I think PY_BT_ERROR should mean that an error definitely
occurred, so a second check isn't needed.
Phil> + gdbpy_print_stack ();
It appears this function will never return PY_BT_ERROR if frame printing
fails.
Phil> + else
Phil> + {
Phil> + Py_DECREF (iterable);
Phil> + error (_("Frame filter must support iteration protocol."));
Why an exception here but not for anything else?
Phil> + self->frame_filters = PyDict_New ();
Phil> + if (!self->frame_filters)
Phil> + {
Phil> + Py_DECREF (self->printers);
I think this will double-free, due to the destructor.
Phil> + Py_DECREF (self->printers);
Phil> + Py_DECREF (self->frame_filters);
Likewise.
Phil> +PyObject *
Phil> +objfpy_get_frame_filters (PyObject *o, void *ignore)
Needs a comment.
Phil> +static int
Phil> +objfpy_set_frame_filters (PyObject *o, PyObject *filters, void *ignore)
Likewise.
Phil> + object->frame_filters = PyDict_New ();
Phil> + if (!object->frame_filters)
Phil> + {
Phil> + Py_DECREF (object->printers);
Double free.
Phil> + Py_DECREF (object->printers);
Phil> + Py_DECREF (object->frame_filters);
Likewise.
Phil> + self->frame_filters = PyDict_New ();
Phil> + if (!self->frame_filters)
Phil> + {
Phil> + Py_DECREF (self->printers);
Likewise.
Phil> + Py_DECREF (self->printers);
Phil> + Py_DECREF (self->frame_filters);
Likewise.
Phil> +PyObject *
Phil> +pspy_get_frame_filters (PyObject *o, void *ignore)
Needs a comment.
Phil> +static int
Phil> +pspy_set_frame_filters (PyObject *o, PyObject *frame, void *ignore)
Likewise.
Phil> + object->frame_filters = PyDict_New ();
Phil> + if (!object->frame_filters)
Phil> + {
Phil> + Py_DECREF (object->printers);
Double free.
Phil> + Py_DECREF (object->printers);
Phil> + Py_DECREF (object->frame_filters);
Likewise.
Phil> +
Phil> set_program_space_data (pspace, pspy_pspace_data_key, object);
Extra blank line.
Phil> +/* This is a cleanup function which decrements the refcount on a
Phil> + Python object. This function accounts appropriately for NULL
Phil> + references. */
Phil> +
Phil> +static void
Phil> +py_xdecref (void *p)
Phil> +{
Phil> + PyObject *py = p;
Phil> +
Phil> + /* Note that we need the extra braces in this 'if' to avoid a
Phil> + warning from gcc. */
Phil> + if (py)
Phil> + {
Phil> + Py_XDECREF (py);
You don't need the 'if'. Py_XDECREF has one already.
Phil> + return make_cleanup (py_xdecref, (void *) py);
You don't need the cast.
Phil> +/* Python frame-filter status returns constants. */
Phil> +static const int PY_BT_ERROR = 0;
Phil> +static const int PY_BT_COMPLETED = 1;
Phil> +static const int PY_BT_NO_FILTERS = 2;
This should be an enum and then various 'int' types in the code changed
as well.
Tom
More information about the Gdb-patches
mailing list