Re: [PATCH 1/2] Use bool in ada_add_all_symbols

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tromey@adacore.com> writes:

> This changes ada_add_all_symbols to use bool, then fixes up the
> callers.
> ---
>  gdb/ada-lang.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 71a338ce17e..afdd59965fd 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -104,7 +104,7 @@ static void ada_add_block_symbols (std::vector<struct block_symbol> &,
>  static void ada_add_all_symbols (std::vector<struct block_symbol> &,
>  				 const struct block *,
>  				 const lookup_name_info &lookup_name,
> -				 domain_search_flags, int, int *);
> +				 domain_search_flags, bool, bool *);
>  
>  static bool is_nonfunction (const std::vector<struct block_symbol> &);
>  
> @@ -5528,7 +5528,7 @@ ada_add_block_renamings (std::vector<struct block_symbol> &result,
>  	  lookup_name_info decl_lookup_name (r_name,
>  					     lookup_name.match_type ());
>  	  ada_add_all_symbols (result, block, decl_lookup_name, domain,
> -			       1, NULL);
> +			       true, nullptr);
>  	}
>      }
>    return result.size () != defns_mark;
> @@ -5612,10 +5612,10 @@ add_nonlocal_symbols (std::vector<struct block_symbol> &result,
>  }
>  
>  /* Find symbols in DOMAIN matching LOOKUP_NAME, in BLOCK and, if
> -   FULL_SEARCH is non-zero, enclosing scope and in global scopes,
> +   FULL_SEARCH is true, enclosing scope and in global scopes,
>     returning the number of matches.  Add these to RESULT.
>  
> -   When FULL_SEARCH is non-zero, any non-function/non-enumeral
> +   When FULL_SEARCH is false, any non-function/non-enumeral

I found the description in this comment for what FULL_SEARCH actually
does really confusing.

I noticed that you replaced non-zero with false not true, which is an
inversion of the logic.  I thought I'd just read the comment and check
the code, then I'd know if this was a fix or not.  But I cannot tell
from the comment what the code is supposed to do, so I'm stuck.

All I know is that the code didn't change, but the comment has flipped
logic, and the commit message doesn't call this out as a fix, so I
wanted to check with you.

>     symbol match within the nest of blocks whose innermost member is BLOCK,
>     is the one match returned (no other matches in that or
>     enclosing blocks is returned).  If there are any matches in or
> @@ -5633,13 +5633,13 @@ ada_add_all_symbols (std::vector<struct block_symbol> &result,
>  		     const struct block *block,
>  		     const lookup_name_info &lookup_name,
>  		     domain_search_flags domain,
> -		     int full_search,
> -		     int *made_global_lookup_p)
> +		     bool full_search,
> +		     bool *made_global_lookup_p)
>  {
>    struct symbol *sym;
>  
> -  if (made_global_lookup_p)
> -    *made_global_lookup_p = 0;
> +  if (made_global_lookup_p != nullptr)
> +    *made_global_lookup_p = false;
>  
>    /* Special case: If the user specifies a symbol name inside package
>       Standard, do a non-wild matching of the symbol name without
> @@ -5679,8 +5679,8 @@ ada_add_all_symbols (std::vector<struct block_symbol> &result,
>        return;
>      }
>  
> -  if (made_global_lookup_p)
> -    *made_global_lookup_p = 1;
> +  if (made_global_lookup_p != nullptr)
> +    *made_global_lookup_p = true;
>  
>    /* Search symbols from all global blocks.  */
>  
> @@ -5694,12 +5694,12 @@ ada_add_all_symbols (std::vector<struct block_symbol> &result,
>  }
>  
>  /* Find symbols in DOMAIN matching LOOKUP_NAME, in BLOCK and, if FULL_SEARCH
> -   is non-zero, enclosing scope and in global scopes.
> +   is true, enclosing scope and in global scopes.
>  
>     Returns (SYM,BLOCK) tuples, indicating the symbols found and the
>     blocks and symbol tables (if any) in which they were found.
>  
> -   When full_search is non-zero, any non-function/non-enumeral
> +   When full_search is false, any non-function/non-enumeral

While here it would be good to capitalise FULL_SEARCH.  Also non-zero
became false here instead of true, as with ada_add_all_symbols, is this
intentional?

The actual code changes look fine though.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew