This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA 21/42] Move the context stack to buildsym_compunit


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]