Re: [RFC] A new command 'explore'

[Joel Brobecker <brobecker at adacore dot com>]

Hi Siva,

> 2011-12-19 Siva Chandra <sivachandra@google.com>
> 
>         New command 'explore' which helps explore values in scope.
>         * data-directory/Makefile.in: Add gdb/command/explore.py
>         * python/lib/gdb/command/explore.py: Implemention of the 'explore'
>         command using the GDB Python API.
>         * testsuite/gdb.python/Makefile.in: Add py-explore to EXECUTABLES
>         * testsuite/gdb.python/py-explore.c: C program used for testing
>         the new 'explore' command.
>         * testsuite/gdb-python/py-explore.exp: Tests for the new 'explore'
>         command.

Thanks for sending this. It's always exciting when you see people
send patches using the extension language. I have a few minor
comments for you... I'll try to skip the comments that Tom already
made.

> +import gdb

GDB already pre-imports module `gdb', and it's documented in the GDB
manual. So I think you can get rid of this import. Though, I wonder
if that's the recommended style or not. Tom?

> +    _SCALAR_TYPE_LIST = [
> +        gdb.TYPE_CODE_CHAR,
> +        gdb.TYPE_CODE_INT,
> +        gdb.TYPE_CODE_BOOL,
> +        gdb.TYPE_CODE_FLT,
> +        gdb.TYPE_CODE_VOID,
> +        gdb.TYPE_CODE_ENUM,
> +    ]

It's not very important, but since the list is constant, make that
a tuple. Generally speaking, I have noticed that people tend to use
lists all the time, and many times a tuple is what they really should
be using. I checked the performance delta between list and tuple a
while back, and it was significant.

> +    @staticmethod
> +    def explore_expr(expr, value, is_child):

I think it would be nice if each method was documented, the same
way we document our code in C.

> +            print ("Explorer for type \'%s\' not yet available.\n" %
                                        ^^^^^^^
I don't think the backslashes are necessary here. Same elsewhere...

> +    def is_scalar_type(type):
> +        if type.code in Explorer._SCALAR_TYPE_LIST:
> +            return True
> +        else:
> +            return False

Nitpick: 
        return type.code in Explorer._SCALAR_TYPE_LIST
?

> +class ScalarExplorer(object):
> +    """Internal class used to explore scalar values."""

It seems that the design is centered around these various *Explorer
classes implementing a defined interface.  If that's the case,
I think it would make it clearer if they inherited from an abstract
class where you can then clearly describe what the interface is,
and what each element is expected to do. Put an assertion in each
abstract method to raise an exception if the method is not implemented
by the deriving class.

Also, almost everything is declared @staticmethod. Is that typical
Python? I don't have enough experience with expert Python to really
say anything, but it almost seems like you're taking the problem
backwards. Let's imagine that you had one abstract Explorer class
which did all the generic stuff, and then a collection of *Explorer
classes which derived from it, you'd then only need a factory that
the commands would invoke, and then the rest would be standard python.
It would avoid all the @staticmethod, as well as the explicit class
names in the method invocations.

This is not a request to change, but just some suggestions based
on my limited understanding of the code and of Python in the general.
This code is self contained, so it does not matter as much how it
is designed. Please do what you think is best.

> +class ExploreUtils(object):
> +    """Internal class which provides utilities for the main classes."""
> +
> +    @staticmethod
> +    def init_env():
> +        Explorer.type_code_to_explorer_map = {
> +            gdb.TYPE_CODE_CHAR : ScalarExplorer,
> +            gdb.TYPE_CODE_INT : ScalarExplorer,
> +            gdb.TYPE_CODE_BOOL : ScalarExplorer,
> +            gdb.TYPE_CODE_FLT : ScalarExplorer,
> +            gdb.TYPE_CODE_VOID : ScalarExplorer,
> +            gdb.TYPE_CODE_ENUM : ScalarExplorer,
> +            gdb.TYPE_CODE_STRUCT : CompoundExplorer,
> +            gdb.TYPE_CODE_UNION : CompoundExplorer,
> +            gdb.TYPE_CODE_PTR : PointerExplorer,
> +            gdb.TYPE_CODE_TYPEDEF : TypedefExplorer,
> +            gdb.TYPE_CODE_ARRAY : ArrayExplorer
> +        }

You wouldn't have to put that static map as a class variable
in class Explorer. This map would be all your factory would need.
I'd make that a pure function, or even a global "constant" that
you'd use from your commands.

> +    def get_type_from_str(type_str):
> +        try:
> +            # Assume the current language to be C/C++ and make a try.
> +            return gdb.parse_and_eval("(%s *)0" % type_str).type.target()
> +        except RuntimeError:
> +            # If assumption of current language to be C/C++ was wrong, then
> +            # lookup the type using the API.
> +            try:
> +                return gdb.lookup_type(type_str)
> +            except RuntimeError:
> +                return None

We really need to think about giving access to parameter values
from python.... That would make the code above a little easier,
as you'd be able to temporarily force the language to C...


-- 
Joel