Re: [RFC][python] Add support for convenience functions implemented in Python

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Thiago" == Thiago Jung Bauermann <bauerman@br.ibm.com> writes:

Thiago> This is a new version of the patch
[...]

I have a couple little nits.

Also I want to point out the memory allocation oddities in this patch
... I am not super concerned by them but I would be interested in
other opinions, if there are any.

Thiago> diff --git a/gdb/parse.c b/gdb/parse.c
Thiago> index eee1f8e..dc2ccb1 100644
[...]
Thiago> -   this is strictly for the convenience of debugging gdb itself.  Gdb
Thiago> +   this is strictly for the convenience of debugging gdb itself.

This is fine but it seems unrelated to this patch.

Thiago> +      error(_("Error while executing Python code."));

Missing a space before the first "(".

Thiago> +      error(_("Error while executing Python code."));

Likewise.

Thiago> +void
Thiago> +gdbpy_initialize_functions (void)
Thiago> +{
Thiago> +  fnpy_object_type.tp_new = PyType_GenericNew;
Thiago> +  fnpy_object_type.tp_init = fnpy_init;

Lately I am preferring to just put these directly into the global's
definition.

Thiago> +value_create_internal_function (const char *name,
[...]
Thiago> +  /* The internal_function object is leaked here -- to make it truly
Thiago> +     deletable, we would have to reference count it and add special
Thiago> +     code to value_free and value_copy.  The setup here is a bit odd
Thiago> +     in general.  It would be better to have a special case in
Thiago> +     help_command.  */

So, this is the primary memory leak.  You can see it in action by
defining a convenience function, then assigning something else to the
variable.  That is, if your function is $foo, invoking "set $foo = 0"
will leak the function definition.

I could implement the reference counting idea if this seems important
to anybody.  I tend to doubt this will happen much -- it isn't common
to define a lot of convenience functions and it is even less common to
delete them.

Another idea would be to make them un-assignable.

Tom