Re: [Patch v18 4/4] Add xmethod support to the Python API

[Doug Evans <xdje42 at gmail dot com>]

Siva Chandra <sivachandra@google.com> writes:

Hi.  Comments inline and at the end.

> diff --git a/gdb/python/lib/gdb/xmethod.py b/gdb/python/lib/gdb/xmethod.py
> new file mode 100644
> index 0000000..9d0deff
> --- /dev/null
> +++ b/gdb/python/lib/gdb/xmethod.py
> @@ -0,0 +1,254 @@
> +# Python side of the support for xmethods.
> +# Copyright (C) 2013-2014 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +"""Utilities for defining xmethods"""
> +
> +import gdb
> +import re
> +import sys
> +
> +
> +if sys.version_info[0] > 2:
> +    # Python 3 removed basestring and long
> +    basestring = str
> +    long = int
> +
> +
> +class XMethod(object):
> +    """Base class (or a prototype) for an xmethod description.
> +
> +    Currently, the description only requires only 'name' and 'enabled'
> +    attributes.  Description objects are managed by 'XMethodMatcher'
> +    objects (see below).

Please add text here that says this class is only used when
XMethodMatcher.methods is used, and that this class specifies the
interface of XMethodMatcher.methods objects.

> +
> +    Attributes:
> +        name: The name of the xmethod.
> +        enabled: A boolean indicating if the xmethod is enabled.
> +    """
> +
> +    def __init__(self, name):
> +        self.name = name
> +        self.enabled = True
> +
> +
> +class XMethodMatcher(object):
> +    """Abstract base class for matching an xmethod.
> +
> +    When looking for xmethods, GDB invokes the `match' method of a
> +    registered xmethod matcher to match the object type and method name.
> +    The `match' method in concrete classes derived from this class should
> +    return an `XMethodWorker' object, or a list of `XMethodWorker'
> +    objects if there is a match (see below for 'XMethodWorker' class).
> +
> +    Attributes:
> +        name: The name of the matcher.
> +        enabled: A boolean indicating if the matcher is enabled.
> +        methods: A sequence of objects of type 'XMethod', or objects
> +            which have at least the attributes of an 'XMethod' object.
> +            This list is used by the 'enable'/'disable'/'info' commands to
> +            enable/disable/list the xmethods registered with GDB.  See
> +            the 'match' method below to know how this sequence is used.
> +    """
> +
> +    def __init__(self, name):
> +        """
> +        Args:
> +            name: An identifying name for the xmethod or the group of
> +                  xmethods returned by the `match' method.
> +        """
> +        self.name = name
> +        self.enabled = True
> +        self.methods = None
> +
> +    def match(self, class_type, method_name):
> +        """Match class type and method name.
> +
> +        In derived classes, it should return an XMethodWorker object, or a
> +        sequence of 'XMethodWorker' objects.  Only those xmethod workers
> +        whose corresponding 'XMethod' descriptor object is enabled should be
> +        returned.
> +
> +        Args:
> +            class_type: The class type (gdb.Type object) to match.
> +            method_name: The name (string) of the method to match.
> +        """
> +        raise NotImplementedError("XMethodMatcher match")
> +
> +
> +class XMethodWorker(object):
> +    """Base class for all xmethod workers defined in Python.
> +
> +    An xmethod worker is an object which matches the method arguments, and
> +    invokes the method when GDB wants it to.  Internally, GDB first invokes the
> +    'get_arg_types' method to perform overload resolution.  If GDB selects to
> +    invoke this Python xmethod, then it invokes it via the overridden
> +    'invoke' method.
> +
> +    Derived classes should override the 'get_arg_types' and 'invoke' methods.
> +    """
> +
> +    def get_arg_types(self):
> +        """Return arguments types of an xmethod.
> +
> +        A sequence of gdb.Type objects corresponding to the arguments of the
> +        xmethod are returned.  If the xmethod takes no arguments, then 'None'
> +        or an empty sequence is returned.  If the xmethod takes only a single
> +        argument, then a gdb.Type object or a sequence with a single gdb.Type
> +        element is returned.
> +        """
> +        raise NotImplementedError("XMethod get_arg_types")
> +
> +    def invoke(self, obj, args):
> +        """Invoke the xmethod.
> +
> +        Args:
> +            obj: The gdb.Value of the object on which the method is to be
> +                 invoked.
> +            args: The tuple of arguments to the method.  Each element of the
> +                  tuple is a gdb.Value object.
> +
> +        Returns:
> +            A gdb.Value corresponding to the value returned by the xmethod.
> +            Returns 'None' if the method does not return anything.
> +        """
> +        raise NotImplementedError("XMethod invoke")
> +
> +
> +class SimpleXMethodMatcher(XMethodMatcher):
> +    """A utility class to implement simple xmethod mathers and workers.
> +
> +    See the __init__ method below for information on how instances of this
> +    class can be used.
> +
> +    For simple classes and methods, one can choose to use this class.  For
> +    complex xmethods, which need to replace/implement template methods on
> +    possibly template classes, one should implement their own xmethod
> +    matchers and workers.  See py-xmethods.py in testsuite/gdb.python
> +    directory of the GDB source tree for examples.
> +    """
> +
> +    class SimpleXMethodWorker(XMethodWorker):
> +        def __init__(self, method_function, arg_types):
> +            self._arg_types = arg_types
> +            self._method_function = method_function
> +
> +        def get_arg_types(self):
> +            return self._arg_types
> +
> +        def invoke(self, obj, args):
> +            return self._method_function(obj, *args)
> +
> +
> +    def __init__(self, name, class_matcher, method_matcher, method_function,
> +                 *arg_types):
> +        """
> +        Args:
> +            name: Name of the xmethod matcher.
> +            class_matcher: A regular expression used to match the name of the
> +                class whose method this xmethod is implementing/replacing.
> +            method_matcher: A regular expression used to match the name of the
> +                method this xmethod is implementing/replacing.
> +            method_function: A Python callable which would be called via the
> +                'invoke' method of the worker returned by the objects of this
> +                class.  This callable should accept the object (*this) as the
> +                first argument followed by the rest of the arguments to the
> +                method. All arguments to this function should be gdb.Value
> +                objects.
> +            arg_types: The gdb.Type objects corresponding to the arguments that
> +                this xmethod takes. It can be None, or an empty sequence,
> +                or a single gdb.Type object, or a sequence of gdb.Type objects.
> +        """
> +        XMethodMatcher.__init__(self, name)
> +        assert callable(method_function), (
> +            "The 'method_function' argument to 'SimpleXMethodMatcher' "
> +            "__init__ method should be a callable.")
> +        self._method_function = method_function
> +        self._class_matcher = class_matcher
> +        self._method_matcher = method_matcher
> +        self._arg_types = arg_types
> +
> +    def match(self, class_type, method_name):
> +        cm = re.match(self._class_matcher, str(class_type.unqualified().tag))
> +        mm = re.match(self._method_matcher, method_name)
> +        if cm and mm:
> +            return SimpleXMethodMatcher.SimpleXMethodWorker(
> +                self._method_function, self._arg_types)

Hi.  Reading this got a bit confusing until I checked the pretty-printer
support. Note that the baseclass XMethodMatcher has an attribute, methods,
that is not used and not needed by derived class SimpleXMethodMatcher.
XMethodMatcher.methods is present for the same reason as
PrettyPrinter.subprinters, documented like this in python/lib/gdb/printing.py:

    # While one might want to push subprinters into the subclass, it's
    # present here to formalize such support to simplify
    # commands/pretty_printers.py.

Please add a comment identical to this one to XMethodMatcher.

Plus,
the description for PrettyPrinter.subprinters has this to say
about the value None"

            Or this is None if there are no subprinters.

Please add identical text to XMethodMatcher.methods.
[s/subprinters/methods/ of course]

---

Another thing occurs to me.
Pretty-printers are invoked, initially, via __call__, not via a "print"
method or some such.
It would be more python-esque if XMethodMatcher.match was renamed __call__
and XMethodWorker.invoke was renamed __call__.

Finally, IWBN if XMethodWorker.__call__ can be written using the same
arg list as the c++ method (instead of the "args" argument that exists
now).  I don't know python well enough to say if this is possible,
but if it is we should do that.
I tried the following simple experiment that gives me hope this is possible.

---snip---
class XMethodWorker(object):
    def __call__(self, obj):
        raise NotImplementedError("XMethodWorker invoke")

class MyXMWorker1(XMethodWorker):
    def __call__(self, obj, arg1):
        return arg1 + 1

class MyXMWorker2(XMethodWorker):
    def __call__(self, obj, arg1, arg2):
        return arg1 + arg2

this = 42
foo1 = MyXMWorker1()
foo2 = MyXMWorker2()

print "%d" % foo1(this, 1)
print "%d" % foo2(this, 1, 2)
---snip---

bash$ python foo.py
2
3
bash$