Re: [PATCH 1/4 v18] Add xmethod documentation and NEWS entry

[Doug Evans <xdje42 at gmail dot com>]

Doug Evans <xdje42@gmail.com> writes:

> Siva Chandra <sivachandra@google.com> writes:
> [...]
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 72d58ad..026790f 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -2327,8 +2327,8 @@ existing matcher with the same name as @code{matcher} if
>  globally.
>  @end defun
>  
> -@node Writing a Xmethod
> -@subsubsection Writing a Xmethod
> +@node Writing an Xmethod
> +@subsubsection Writing an Xmethod
>  @cindex writing xmethods in Python
>  
>  Implementing xmethods in Python will require implementing xmethod
> @@ -2372,6 +2372,11 @@ class MyClassMatcher(gdb.xmethod.XMethodMatcher):
>      def match(self, class_type, method_name):
>          if class_type.tag != 'MyClass':
>              return None
> +        # FIXME: User code shouldn't have to check the enabled flag.
> +        # py-xmethods.c checks it, so can this check just be removed?
> +        # If user code wants to add this enabled check, e.g., should a perf
> +        # concern arise, then ok; but we shouldn't encourage it as the default
> +        # mode of writing xmethods.
>          if method_name == 'geta' and self.methods[0].enabled:
>              return MyClassWorker_geta()
>          elif method_name == 'operator+' and self.methods[1].enabled:
> @@ -2386,6 +2391,7 @@ a worker object of type @code{MyClassWorker_geta} for the @code{geta}
>  method, and a worker object of type @code{MyClassWorker_plus} for the
>  @code{operator+} method.  Also, a worker object is returned only if the
>  corresponding entry in the @code{methods} attribute is enabled.
> +@c FIXME: See above: User code shouldn't check enabled flag, gdb should.
>  
>  The implementation of the worker classes returned by the matcher above
>  is as follows:

Ok, I was wrong here.
What's going on here is equivalent to what
python/lib/gdb/printing.y:RegexpCollectionPrettyPrinter is doing here:

        # Iterate over table of type regexps to determine
        # if a printer is registered for that type.
        # Return an instantiation of the printer if found.
        for printer in self.subprinters:
            if printer.enabled and printer.compiled_re.search(typename):
                return printer.gen_printer(val)

However, note that in the pretty-printer case the result is obtained by
invoking a method on the subprinter:

                return printer.gen_printer(val)

whereas in the MyClassMatcher case all self.methods is used for is the
enabled flag:

        if method_name == 'geta' and self.methods[0].enabled:
            return MyClassWorker_geta()
        elif method_name == 'operator+' and self.methods[1].enabled:
            return MyClassWorker_plus()
        else:
            return None

It would be cleaner to have an example where this code looked
something like the pretty-printer case:

        for method in self.methods:
            if method.enabled and method.name == method_name
                return method.gen_worker()
        return None

Maybe something like:

class MyClassMatcher(gdb.xmethod.XMethodMatcher):
    class MyClassMethod(gdb.xmethod.XMethod):
        def __init(self, name, worker):
        gdb.xmethod.XMethod.__init__(self, name)
        self.worker = worker

    def __init__(self):
        gdb.xmethod.XMethodMatcher.__init__(self, 'MyMatcher')
        # List of methods 'managed' by this matcher
        self.methods = [MyClassMethod('geta', MyClassWorker_geta()),
                        MyClassXMethod('operator+', MyClassWorker_plus())]

    def __call__(self, class_type, method_name):
        if class_type.tag != 'MyClass':
            return None
        for method in self.methods:
            if method.enabled and method.name == method_name
                return method.worker
        return None

?