Re: [PATCH 2/9] Define and export Guile classes for all GDB object types

[Doug Evans <xdje42 at gmail dot com>]

Andy Wingo <wingo@igalia.com> writes:

> * gdb/guile/scm-gsmob.c (gdbscm_make_smob_type): Define a binding for a
>   GOOPS class corresponding to the SMOB type.  In Guile 2.0, this
>   binding is also exported by (oop goops), but this is no longer the
>   case in Guile 2.2, so we take care of doing that here.
>   (gdbscm_initialize_smobs): Load GOOPS, so that we can ensure the
>   classes actually get created.
>
> * gdb/guile/lib/gdb.scm: Export the GOOPS classes.
>
> * gdb/testsuite/gdb.guile/scm-generics.exp: Import (gdb) in the test so
>   that we have access to the <gdb:value> type in Guile 2.2.
> ---
>  gdb/guile/lib/gdb.scm                    | 18 ++++++++++++++++++
>  gdb/guile/scm-gsmob.c                    | 14 +++++++++++++-
>  gdb/testsuite/gdb.guile/scm-generics.exp |  2 +-
>  3 files changed, 32 insertions(+), 2 deletions(-)

Hi.
ChangeLog format issues covered in 1/9 so I won't repeat them here
(or for 3-9), except to say comments explaining "why things are the
way they are" go in the code, not the ChangeLog.
The ChangeLog entry is just a brief description of what was changed.
But feel free to add whatever extensive elaboration
you desire in the git commit log entry.

[I can imagine the above is more of the latter, which is fine, except
that comments in the code are still required.  And a properly formed
ChangeLog entry is also still required, at least until community comes
to an agreement on what changes they want to make.  I realize Guile
does things differently, but until the gdb community agrees on changes
I'm obligated to ask for adherence to existing conventions.]

> diff --git a/gdb/guile/lib/gdb.scm b/gdb/guile/lib/gdb.scm
> index f12769e..37f0934 100644
> --- a/gdb/guile/lib/gdb.scm
> +++ b/gdb/guile/lib/gdb.scm
> @@ -278,6 +278,24 @@
>   gsmob-has-property?
>   gsmob-properties
>  
> + <gdb:value>
> + <gdb:block>
> + <gdb:iterator>
> + <gdb:pretty-printer-worker>
> + <gdb:pretty-printer>
> + <gdb:sal>
> + <gdb:symtab>
> + <gdb:frame>
> + <gdb:block-symbols-iterator>
> + <gdb:field>
> + <gdb:type>
> + <gdb:arch>
> + <gdb:exception>
> + <gdb:objfile>
> + <gdb:lazy-string>
> + <gdb:breakpoint>
> + <gdb:symbol>
> +
>   ;; scm-string.c
>  
>   string->argv

The export list is organized by the file the symbols come from.
I think it would be useful to preserve that.

> diff --git a/gdb/guile/scm-gsmob.c b/gdb/guile/scm-gsmob.c
> index b0f9e19..4c88ff9 100644
> --- a/gdb/guile/scm-gsmob.c
> +++ b/gdb/guile/scm-gsmob.c
> @@ -120,7 +120,17 @@ gdbscm_is_gsmob (SCM scm)
>  scm_t_bits
>  gdbscm_make_smob_type (const char *name, size_t size)
>  {
> -  scm_t_bits result = scm_make_smob_type (name, size);
> +  scm_t_bits result;
> +  SCM klass;
> +  char *class_name;
> +
> +  result = scm_make_smob_type (name, size);
> +
> +  klass = scm_smob_class[SCM_TC2SMOBNUM (result)];
> +  gdb_assert (SCM_UNPACK (klass) != 0);
> +  class_name = xstrprintf ("<%s>", name);
> +  scm_c_define (class_name, klass);
> +  xfree (class_name);

IWBN to document here why this code is needed for 2.2 and wasn't for 2.0.

>  
>    register_gsmob (result);
>    return result;
> @@ -475,6 +485,8 @@ Return an unsorted list of names of properties." },
>  void
>  gdbscm_initialize_smobs (void)
>  {
> +  scm_c_use_module ("oop goops");

Please add a comment documenting why the use_module is needed here.

> +
>    registered_gsmobs = htab_create_alloc (10,
>  					 hash_scm_t_bits, eq_scm_t_bits,
>  					 NULL, xcalloc, xfree);
> diff --git a/gdb/testsuite/gdb.guile/scm-generics.exp b/gdb/testsuite/gdb.guile/scm-generics.exp
> index 664affc..93ab0e5 100644
> --- a/gdb/testsuite/gdb.guile/scm-generics.exp
> +++ b/gdb/testsuite/gdb.guile/scm-generics.exp
> @@ -30,7 +30,7 @@ gdb_reinitialize_dir $srcdir/$subdir
>  gdb_install_guile_utils
>  gdb_install_guile_module
>  
> -gdb_test_no_output "guile (use-modules ((oop goops)))"
> +gdb_test_no_output "guile (use-modules (oop goops) (gdb))"

gdb_install_guile_module has already imported the gdb module.
Is there an ordering dependency? [seems unlikely, but I may be missing
something]

>  
>  gdb_test_no_output "guile (define-generic +)"
>  gdb_test_no_output "guile (define-method (+ (x <gdb:value>) (y <gdb:value>)) (value-add x y))"

Thanks!