This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA 21/42] Move the context stack to buildsym_compunit
- From: Keith Seitz <keiths at redhat dot com>
- To: Tom Tromey <tom at tromey dot com>, gdb-patches at sourceware dot org
- Date: Fri, 6 Jul 2018 10:30:48 -0700
- Subject: Re: [RFA 21/42] Move the context stack to buildsym_compunit
- References: <20180523045851.11660-1-tom@tromey.com> <20180523045851.11660-22-tom@tromey.com>
On 05/22/2018 09:58 PM, Tom Tromey wrote:
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index 4e3ca5e4b1..54592b7795 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -213,6 +213,9 @@ struct buildsym_compunit
> /* global "using" directives. */
>
> struct using_direct *m_global_using_directives = nullptr;
> +
> + std::vector<struct context_stack> m_context_stack;
> + struct context_stack m_popped_context {};
> };
Explanatory comments -- especially for m_popped_context? Had I not read the commit log, I would not have (as easily) understood why this was necessary.
> /* The work-in-progress of the compunit we are building.
> @@ -1583,12 +1579,9 @@ augment_type_symtab (void)
> struct compunit_symtab *cust = buildsym_compunit->compunit_symtab;
> const struct blockvector *blockvector = COMPUNIT_BLOCKVECTOR (cust);
>
> - if (context_stack_depth > 0)
> - {
> - complaint (&symfile_complaints,
> - _("Context stack not empty in augment_type_symtab"));
> - context_stack_depth = 0;
> - }
> + if (!buildsym_compunit->m_context_stack.empty ())
> + complaint (&symfile_complaints,
> + _("Context stack not empty in augment_type_symtab"));
> if (pending_blocks != NULL)
> complaint (&symfile_complaints, _("Blocks in a type symtab"));
> if (buildsym_compunit->m_pending_macros != NULL)
Warning: This patch (and one or two others) need rebasing. symfile_complaints was removed in May.
> @@ -1745,6 +1736,35 @@ get_global_using_directives ()
> return &buildsym_compunit->m_global_using_directives;
> }
>
> +/* See buildsym.h. */
> +
> +bool
> +outermost_context_p ()
> +{
> + gdb_assert (buildsym_compunit != nullptr);
> + return buildsym_compunit->m_context_stack.empty ();
> +}
> +
> +/* See buildsym.h. */
> +
> +struct context_stack *
> +get_current_context_stack ()
> +{
> + gdb_assert (buildsym_compunit != nullptr);
> + if (buildsym_compunit->m_context_stack.empty ())
> + return nullptr;
> + return &buildsym_compunit->m_context_stack.back ();
> +}
> +
> +/* See buildsym.h. */
> +
> +int
> +get_context_stack_depth ()
> +{
> + gdb_assert (buildsym_compunit != nullptr);
> + return buildsym_compunit->m_context_stack.size ();
> +}
> +
I can't help but think these function names, which could appear in several source files, seem a little vague or collision-prone (conceptually)... Would it be worth it for these (and perhaps all similar buildsym.h get_* functions) to be namespaced or further prefixed, e.g., buildsym_[get_]current_context_stack or buildsym::get_current_context_stack? I'm not going to suggest [Dare I say, "require?"] any changes -- I'm just wondering "aloud."
> /* Initialize anything that needs initializing when starting to read a
> diff --git a/gdb/buildsym.h b/gdb/buildsym.h
> index efb35c907b..fe158d183c 100644
> --- a/gdb/buildsym.h
> +++ b/gdb/buildsym.h
> @@ -270,6 +261,19 @@ extern void set_local_using_directives (struct using_direct *new_local);
>
> extern struct using_direct **get_global_using_directives ();
>
> +/* Non-zero if the context stack is empty. */
^^^^^^^^
> +
> +extern bool outermost_context_p ();
Since this (now) returns bool, please use boolean in comment. [I realize that's a cut-n-paste-o.]
> +
> +/* Return the top of the context stack, or nullptr if there is
> + entry. */
> +
> +extern struct context_stack *get_current_context_stack ();
> +
Typo in "if there is entry."
> +/* Return the context stack depth. */
> +
> +extern int get_context_stack_depth ();
> +
> #undef EXTERN
>
> #endif /* defined (BUILDSYM_H) */
Thank you!
[Just a gentle reminder: IANAM.]
Keith