diff mbox

[RFA,05/13] Remove directive-searched cleanups

Message ID 20171102223612.3642-6-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey Nov. 2, 2017, 10:36 p.m. UTC 
  This removes a few cleanups related to the "searched" field in
struct using_direct, replacing these with scoped_restore.

gdb/ChangeLog
2017-11-02  Tom Tromey  <tom@tromey.com>

	* cp-namespace.c (reset_directive_searched): Remove.
	(cp_lookup_symbol_via_imports): Use scoped_restore.
	* cp-support.c (reset_directive_searched): Remove.
	(make_symbol_overload_list_using): Use scoped_restore.
	* d-namespace.c (d_lookup_symbol_imports): Use scoped_restore.
	(reset_directive_searched): Remove.
---
 gdb/ChangeLog      |  9 +++++++++
 gdb/cp-namespace.c | 25 +++----------------------
 gdb/cp-support.c   | 19 ++-----------------
 gdb/d-namespace.c  | 26 +++-----------------------
 4 files changed, 17 insertions(+), 62 deletions(-)

Comments

Simon Marchi Nov. 3, 2017, 1:09 a.m. UTC | #1 
On 2017-11-02 18:36, Tom Tromey wrote:
> This removes a few cleanups related to the "searched" field in
> struct using_direct, replacing these with scoped_restore.
> 
> gdb/ChangeLog
> 2017-11-02  Tom Tromey  <tom@tromey.com>
> 
> 	* cp-namespace.c (reset_directive_searched): Remove.
> 	(cp_lookup_symbol_via_imports): Use scoped_restore.
> 	* cp-support.c (reset_directive_searched): Remove.
> 	(make_symbol_overload_list_using): Use scoped_restore.
> 	* d-namespace.c (d_lookup_symbol_imports): Use scoped_restore.
> 	(reset_directive_searched): Remove.
> ---
>  gdb/ChangeLog      |  9 +++++++++
>  gdb/cp-namespace.c | 25 +++----------------------
>  gdb/cp-support.c   | 19 ++-----------------
>  gdb/d-namespace.c  | 26 +++-----------------------
>  4 files changed, 17 insertions(+), 62 deletions(-)
> 
> diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
> index 214b7e1cf4..d8817c0372 100644
> --- a/gdb/cp-namespace.c
> +++ b/gdb/cp-namespace.c
> @@ -338,15 +338,6 @@ cp_lookup_symbol_in_namespace (const char
> *the_namespace, const char *name,
>    return sym;
>  }
> 
> -/* Used for cleanups to reset the "searched" flag in case of an error. 
>  */
> -
> -static void
> -reset_directive_searched (void *data)
> -{
> -  struct using_direct *direct = (struct using_direct *) data;
> -  direct->searched = 0;
> -}
> -
>  /* Search for NAME by applying all import statements belonging to
>     BLOCK which are applicable in SCOPE.  If DECLARATION_ONLY the
>     search is restricted to using declarations.
> @@ -388,7 +379,6 @@ cp_lookup_symbol_via_imports (const char *scope,
>    struct block_symbol sym;
>    int len;
>    int directive_match;
> -  struct cleanup *searched_cleanup;
> 
>    sym.symbol = NULL;
>    sym.block = NULL;
> @@ -425,9 +415,8 @@ cp_lookup_symbol_via_imports (const char *scope,
>  	{
>  	  /* Mark this import as searched so that the recursive call
>  	     does not search it again.  */
> -	  current->searched = 1;
> -	  searched_cleanup = make_cleanup (reset_directive_searched,
> -					   current);
> +	  scoped_restore reset_directive_searched
> +	    = make_scoped_restore (&current->searched, 1);
> 
>  	  /* If there is an import of a single declaration, compare the
>  	     imported declaration (after optional renaming by its alias)
> @@ -446,9 +435,6 @@ cp_lookup_symbol_via_imports (const char *scope,
>  	     search of this import is complete.  */
>  	  if (declaration_only || sym.symbol != NULL || current->declaration)
>  	    {
> -	      current->searched = 0;
> -	      discard_cleanups (searched_cleanup);
> -
>  	      if (sym.symbol != NULL)
>  		return sym;
> 
> @@ -460,10 +446,7 @@ cp_lookup_symbol_via_imports (const char *scope,
>  	    if (strcmp (name, *excludep) == 0)
>  	      break;
>  	  if (*excludep)
> -	    {
> -	      discard_cleanups (searched_cleanup);
> -	      continue;
> -	    }
> +	    continue;

In this case, the cleanup is discarded.  Shouldn't the same thing happen 
with the scoped_restore?  Or was it an error in the original code, and 
we always want to reset searched?

There's the same case in d-namespace.c.

Simon
Tom Tromey Nov. 3, 2017, 4:42 p.m. UTC | #2 
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

>> if (*excludep)
>> -	    {
>> -	      discard_cleanups (searched_cleanup);
>> -	      continue;
>> -	    }
>> +	    continue;

Simon> In this case, the cleanup is discarded.  Shouldn't the same thing
Simon> happen with the scoped_restore?  Or was it an error in the original
Simon> code, and we always want to reset searched?

Simon> There's the same case in d-namespace.c.

I'm afraid I hadn't noticed this.  Ouch.

However, I think it has to be a latent bug, because it will result in
the using_direct's searched flag being permanently set -- nothing else
will ever clear it.

Tom
Simon Marchi Nov. 3, 2017, 4:46 p.m. UTC | #3 
On 2017-11-03 12:42, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
>>> if (*excludep)
>>> -	    {
>>> -	      discard_cleanups (searched_cleanup);
>>> -	      continue;
>>> -	    }
>>> +	    continue;
> 
> Simon> In this case, the cleanup is discarded.  Shouldn't the same 
> thing
> Simon> happen with the scoped_restore?  Or was it an error in the 
> original
> Simon> code, and we always want to reset searched?
> 
> Simon> There's the same case in d-namespace.c.
> 
> I'm afraid I hadn't noticed this.  Ouch.
> 
> However, I think it has to be a latent bug, because it will result in
> the using_direct's searched flag being permanently set -- nothing else
> will ever clear it.
> 
> Tom

That's what my assumption was.  From what I understand, we want to set 
the flag while we recurse and always restore it when we return.

Simon
diff mbox

Patch

diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index 214b7e1cf4..d8817c0372 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -338,15 +338,6 @@  cp_lookup_symbol_in_namespace (const char *the_namespace, const char *name,
   return sym;
 }
 
-/* Used for cleanups to reset the "searched" flag in case of an error.  */
-
-static void
-reset_directive_searched (void *data)
-{
-  struct using_direct *direct = (struct using_direct *) data;
-  direct->searched = 0;
-}
-
 /* Search for NAME by applying all import statements belonging to
    BLOCK which are applicable in SCOPE.  If DECLARATION_ONLY the
    search is restricted to using declarations.
@@ -388,7 +379,6 @@  cp_lookup_symbol_via_imports (const char *scope,
   struct block_symbol sym;
   int len;
   int directive_match;
-  struct cleanup *searched_cleanup;
 
   sym.symbol = NULL;
   sym.block = NULL;
@@ -425,9 +415,8 @@  cp_lookup_symbol_via_imports (const char *scope,
 	{
 	  /* Mark this import as searched so that the recursive call
 	     does not search it again.  */
-	  current->searched = 1;
-	  searched_cleanup = make_cleanup (reset_directive_searched,
-					   current);
+	  scoped_restore reset_directive_searched
+	    = make_scoped_restore (&current->searched, 1);
 
 	  /* If there is an import of a single declaration, compare the
 	     imported declaration (after optional renaming by its alias)
@@ -446,9 +435,6 @@  cp_lookup_symbol_via_imports (const char *scope,
 	     search of this import is complete.  */
 	  if (declaration_only || sym.symbol != NULL || current->declaration)
 	    {
-	      current->searched = 0;
-	      discard_cleanups (searched_cleanup);
-
 	      if (sym.symbol != NULL)
 		return sym;
 
@@ -460,10 +446,7 @@  cp_lookup_symbol_via_imports (const char *scope,
 	    if (strcmp (name, *excludep) == 0)
 	      break;
 	  if (*excludep)
-	    {
-	      discard_cleanups (searched_cleanup);
-	      continue;
-	    }
+	    continue;
 
 	  if (current->alias != NULL
 	      && strcmp (name, current->alias) == 0)
@@ -484,8 +467,6 @@  cp_lookup_symbol_via_imports (const char *scope,
 						  name, block,
 						  domain, 1, 0, 0);
 	    }
-	  current->searched = 0;
-	  discard_cleanups (searched_cleanup);
 
 	  if (sym.symbol != NULL)
 	    return sym;
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 37b2b4af97..817de59f12 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -1286,16 +1286,6 @@  make_symbol_overload_list_adl (struct type **arg_types, int nargs,
   return sym_return_val;
 }
 
-/* Used for cleanups to reset the "searched" flag in case of an
-   error.  */
-
-static void
-reset_directive_searched (void *data)
-{
-  struct using_direct *direct = (struct using_direct *) data;
-  direct->searched = 0;
-}
-
 /* This applies the using directives to add namespaces to search in,
    and then searches for overloads in all of those namespaces.  It
    adds the symbols found to sym_return_val.  Arguments are as in
@@ -1332,16 +1322,11 @@  make_symbol_overload_list_using (const char *func_name,
 	  {
 	    /* Mark this import as searched so that the recursive call
 	       does not search it again.  */
-	    struct cleanup *old_chain;
-	    current->searched = 1;
-	    old_chain = make_cleanup (reset_directive_searched,
-				      current);
+	    scoped_restore reset_directive_searched
+	      = make_scoped_restore (&current->searched, 1);
 
 	    make_symbol_overload_list_using (func_name,
 					     current->import_src);
-
-	    current->searched = 0;
-	    discard_cleanups (old_chain);
 	  }
       }
 
diff --git a/gdb/d-namespace.c b/gdb/d-namespace.c
index bc791f70dc..6842a29fac 100644
--- a/gdb/d-namespace.c
+++ b/gdb/d-namespace.c
@@ -356,16 +356,6 @@  d_lookup_nested_symbol (struct type *parent_type,
     }
 }
 
-/* Used for cleanups to reset the "searched" flag incase
-   of an error.  */
-
-static void
-reset_directive_searched (void *data)
-{
-  struct using_direct *direct = (struct using_direct *) data;
-  direct->searched = 0;
-}
-
 /* Search for NAME by applying all import statements belonging to
    BLOCK which are applicable in SCOPE.  */
 
@@ -376,7 +366,6 @@  d_lookup_symbol_imports (const char *scope, const char *name,
 {
   struct using_direct *current;
   struct block_symbol sym;
-  struct cleanup *searched_cleanup;
 
   /* First, try to find the symbol in the given module.  */
   sym = d_lookup_symbol_in_module (scope, name, block, domain, 1);
@@ -399,9 +388,8 @@  d_lookup_symbol_imports (const char *scope, const char *name,
 	{
 	  /* Mark this import as searched so that the recursive call
 	     does not search it again.  */
-	  current->searched = 1;
-	  searched_cleanup = make_cleanup (reset_directive_searched,
-					   current);
+	  scoped_restore restore_searched
+	    = make_scoped_restore (&current->searched, 1);
 
 	  /* If there is an import of a single declaration, compare the
 	     imported declaration (after optional renaming by its alias)
@@ -419,9 +407,6 @@  d_lookup_symbol_imports (const char *scope, const char *name,
 	     declaration, the search of this import is complete.  */
 	  if (sym.symbol != NULL || current->declaration)
 	    {
-	      current->searched = 0;
-	      discard_cleanups (searched_cleanup);
-
 	      if (sym.symbol != NULL)
 		return sym;
 
@@ -433,10 +418,7 @@  d_lookup_symbol_imports (const char *scope, const char *name,
 	    if (strcmp (name, *excludep) == 0)
 	      break;
 	  if (*excludep)
-	    {
-	      discard_cleanups (searched_cleanup);
-	      continue;
-	    }
+	    continue;
 
 	  /* If the import statement is creating an alias.  */
 	  if (current->alias != NULL)
@@ -476,8 +458,6 @@  d_lookup_symbol_imports (const char *scope, const char *name,
 	      sym = d_lookup_symbol_in_module (current->import_src,
 					       name, block, domain, 1);
 	    }
-	  current->searched = 0;
-	  discard_cleanups (searched_cleanup);
 
 	  if (sym.symbol != NULL)
 	    return sym;