This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 4/7] Move struct varobj to varobj.h.
- From: Doug Evans <dje at google dot com>
- To: Joel Brobecker <brobecker at adacore dot com>
- Cc: Yao Qi <yao at codesourcery dot com>, gdb-patches at sourceware dot org
- Date: Tue, 8 Oct 2013 14:03:16 -0700
- Subject: Re: [PATCH 4/7] Move struct varobj to varobj.h.
- Authentication-results: sourceware.org; auth=none
- References: <1379512482-31773-1-git-send-email-yao at codesourcery dot com> <1379512482-31773-5-git-send-email-yao at codesourcery dot com> <20131002094636 dot GC2971 at adacore dot com> <525103F5 dot 90607 at codesourcery dot com> <20131008045620 dot GD3092 at adacore dot com>
Joel Brobecker writes:
> > 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.
Yeah, except for the nits you found, looks good to me.
[I can image more structs will get moved into headers in a c++ world,
thus this doesn't bother me.]
> 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.
The more lines of code that follow, the more I like the blank line.
For something like:
/* blah blah blah ... */
int foo = 42;
I think the blank line is unnecessary.
[even if the comment itself is several lines]
But for a struct definition where more lines of code are involved, I like
the blank line.
[There is such a rule for function comments. Whether it was originally
created because functions tend to involve several lines ... I'm not sure,
but I do like it, and am glad we're now enforcing it!]