Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Joel Brobecker <brobecker@adacore.com>
To: Yao Qi <yao@codesourcery.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH 4/7] Move struct varobj to varobj.h.
Date: Tue, 08 Oct 2013 04:56:00 -0000	[thread overview]
Message-ID: <20131008045620.GD3092@adacore.com> (raw)
In-Reply-To: <525103F5.90607@codesourcery.com>

> How about this?

It does look a lot better to me, FWIW.  The only possibly contentious
question left would be making struct varobj public, when I personally
tend to prefer opaque structures. But I'm fine with this step, as it
helps achieve the goal of moving the language-specific stuff out of
varobj.c. I think Doug also pretty much agreed to that change. I would
give the patch, say, until the end of the week, JIC others want to
comment in.

How does this new patch affect the rest of the patch series? No effect?
If not, we can continue reviewing the remainder.  Otherwise, can you
post an update?  Sorry it's taking so long. I just don't have much time.
But as I said, I like the direction this is taking.

Thanks!

> 2013-10-06  Yao Qi  <yao@codesourcery.com>
> 
> 	* varobj.c (struct varobj): Move most of the fields to
> 	varobj.h.
> 	(struct varobj_dynamic): New struct.
> 	(varobj_get_display_hint) [HAVE_PYTHON]: Adjust.
> 	(varobj_has_more): Likewise.
> 	(dynamic_varobj_has_child_method): Likewise.
> 	(update_dynamic_varobj_children): Likewise.
> 	(varobj_get_num_children): Likewise.
> 	(varobj_list_children, varobj_pretty_printed_p): Likewise.
> 	(install_new_value_visualizer): Likewise.
> 	(install_new_value_visualizer, install_new_value): Likewise.
> 	(varobj_update, new_variable, free_variable): Likewise.
> 	(my_value_of_variable, value_get_print_value): Likewise.
> 	(install_visualizer): Change the type of parameter 'var' to
> 	'struct varobjd_dynamic *'.  Callers update.
> 	* varobj.h (struct varobj): Moved from varobj.c.
> 	(struct varobj) <dynamic>: New field.

> @@ -2924,7 +2861,7 @@ value_get_print_value (struct value *value, enum varobj_display_formats format,
>  #if HAVE_PYTHON
>    if (gdb_python_initialized)
>      {
> -      PyObject *value_formatter = var->pretty_printer;
> +      PyObject *value_formatter=  var->dynamic->pretty_printer;

You accidently removed a space before '='.

> +/* Every variable in the system has a structure of this type defined
> +   for it.  This structure holds all information necessary to manipulate
> +   a particular object variable.  Members which must be freed are noted.  */
> +struct varobj
> +{

Not sure if there is a rule for it, or not. But I tend to prefer an
empty line between documentation and structure as well (same as with
subprograms). Add it if you agree, or else feel free to ignore. This
is just an arbitrary preference, AFAIK, and it really does not matter
much to me.

-- 
Joel


  reply	other threads:[~2013-10-08  4:56 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-09-18 13:55 [RFC 0/7] Move language-related stuff out of varobj.c Yao Qi
2013-09-18 13:55 ` [PATCH 5/7] New lang-varobj.h Yao Qi
2013-10-02 17:18   ` Doug Evans
2013-10-08  4:59     ` Joel Brobecker
2013-10-09 23:51       ` Yao Qi
2013-10-09 23:56         ` Doug Evans
2013-10-10  0:19           ` Yao Qi
2013-09-18 13:55 ` [PATCH 4/7] Move struct varobj to varobj.h Yao Qi
2013-10-02  9:46   ` Joel Brobecker
2013-10-02 19:32     ` Doug Evans
2013-10-06  6:33     ` Yao Qi
2013-10-08  4:56       ` Joel Brobecker [this message]
2013-10-08 21:03         ` Doug Evans
2013-10-09  0:28         ` Yao Qi
2013-10-14  8:19         ` Yao Qi
2013-09-18 13:55 ` [PATCH 6/7] Move language stuff out of varobj.c Yao Qi
2013-10-11  8:20   ` Yao Qi
2013-10-17  5:40     ` Joel Brobecker
2013-10-17 13:33       ` Yao Qi
2013-09-18 13:55 ` [PATCH 1/7] Remove field language in struct language_specific Yao Qi
2013-10-01 10:03   ` Joel Brobecker
2013-10-01 13:35     ` Yao Qi
2013-09-18 13:55 ` [PATCH 3/7] Remove field value_of_root Yao Qi
2013-10-01 10:16   ` Joel Brobecker
2013-10-01 13:52     ` Yao Qi
2013-09-18 13:55 ` [PATCH 7/7] Remove ada-varobj.h Yao Qi
2013-10-17  5:46   ` Joel Brobecker
2013-10-17 13:34     ` Yao Qi
2013-09-18 13:55 ` [PATCH 2/7] Remove vlang_unknown Yao Qi
2013-10-01 10:07   ` Joel Brobecker
2013-10-01 13:34     ` Yao Qi
2013-10-02  0:19       ` Doug Evans
2013-10-02  9:32         ` Joel Brobecker
2013-10-04  8:31           ` Yao Qi
2013-09-28  0:56 ` [RFC 0/7] Move language-related stuff out of varobj.c Yao Qi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20131008045620.GD3092@adacore.com \
    --to=brobecker@adacore.com \
    --cc=gdb-patches@sourceware.org \
    --cc=yao@codesourcery.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox