[2/4] Simplify resolve_subexp by using C++ algorithms

Message ID 20210218180430.326137-3-tromey@adacore.com
State New
Headers show
Series
  • Minor C++-ifications for Ada
Related show

Commit Message

Tom Tromey Feb. 18, 2021, 6:04 p.m.
This changes resolve_subexp to use any_of and the erase-remove idiom
to simplify the code somewhat.  This simplifies the next patch a bit.

gdb/ChangeLog
2021-02-18  Tom Tromey  <tromey@adacore.com>

	* ada-lang.c (resolve_subexp): Use any_of and erase-remove idiom.
---
 gdb/ChangeLog  |  4 ++++
 gdb/ada-lang.c | 57 +++++++++++++++++++++++++-------------------------
 2 files changed, 32 insertions(+), 29 deletions(-)

-- 
2.26.2

Comments

Andrew Burgess Feb. 19, 2021, 5:55 p.m. | #1
* Tom Tromey <tromey@adacore.com> [2021-02-18 11:04:28 -0700]:

> This changes resolve_subexp to use any_of and the erase-remove idiom

> to simplify the code somewhat.  This simplifies the next patch a bit.

> 

> gdb/ChangeLog

> 2021-02-18  Tom Tromey  <tromey@adacore.com>

> 

> 	* ada-lang.c (resolve_subexp): Use any_of and erase-remove idiom.

> ---

>  gdb/ChangeLog  |  4 ++++

>  gdb/ada-lang.c | 57 +++++++++++++++++++++++++-------------------------

>  2 files changed, 32 insertions(+), 29 deletions(-)

> 

> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c

> index e2b2e6105b0..db4f3591131 100644

> --- a/gdb/ada-lang.c

> +++ b/gdb/ada-lang.c

> @@ -3660,41 +3660,40 @@ resolve_subexp (expression_up *expp, int *pos, int deprocedure_p,

>  	    ada_lookup_symbol_list (exp->elts[pc + 2].symbol->linkage_name (),

>  				    exp->elts[pc + 1].block, VAR_DOMAIN,

>  				    &candidates);

> +	  /* Paranoia.  */

> +	  candidates.resize (n_candidates);


It's not clear what you're being paranoid about here?  I would have
expected that:

   gdb_assert (candidates.size () == n_candidates);

based on the comments on ada_lookup_symbol_list.  But even if that's
not true I can't see what harm would occur if the size and
n_candidates were out of sync.

Could you expand the command here to help those of us who are slower
to catch on :)

Otherwise, I didn't try to confirm the code does exactly as before, I
figure testing will do that, but the new code look very readable, so
LGTM.

Thanks,
Andrew

>  

> -	  if (n_candidates > 1)

> +	  if (std::any_of (candidates.begin (),

> +			   candidates.end (),

> +			   [] (block_symbol &sym)

> +			   {

> +			     switch (SYMBOL_CLASS (sym.symbol))

> +			       {

> +			       case LOC_REGISTER:

> +			       case LOC_ARG:

> +			       case LOC_REF_ARG:

> +			       case LOC_REGPARM_ADDR:

> +			       case LOC_LOCAL:

> +			       case LOC_COMPUTED:

> +				 return true;

> +			       default:

> +				 return false;

> +			       }

> +			   }))

>  	    {

>  	      /* Types tend to get re-introduced locally, so if there

>  		 are any local symbols that are not types, first filter

>  		 out all types.  */

> -	      int j;

> -	      for (j = 0; j < n_candidates; j += 1)

> -		switch (SYMBOL_CLASS (candidates[j].symbol))

> +	      candidates.erase

> +		(std::remove_if

> +		 (candidates.begin (),

> +		  candidates.end (),

> +		  [] (block_symbol &sym)

>  		  {

> -		  case LOC_REGISTER:

> -		  case LOC_ARG:

> -		  case LOC_REF_ARG:

> -		  case LOC_REGPARM_ADDR:

> -		  case LOC_LOCAL:

> -		  case LOC_COMPUTED:

> -		    goto FoundNonType;

> -		  default:

> -		    break;

> -		  }

> -	    FoundNonType:

> -	      if (j < n_candidates)

> -		{

> -		  j = 0;

> -		  while (j < n_candidates)

> -		    {

> -		      if (SYMBOL_CLASS (candidates[j].symbol) == LOC_TYPEDEF)

> -			{

> -			  candidates[j] = candidates[n_candidates - 1];

> -			  n_candidates -= 1;

> -			}

> -		      else

> -			j += 1;

> -		    }

> -		}

> +		    return SYMBOL_CLASS (sym.symbol) == LOC_TYPEDEF;

> +		  }),

> +		 candidates.end ());

> +	      n_candidates = candidates.size ();

>  	    }

>  

>  	  if (n_candidates == 0)

> -- 

> 2.26.2

>
Andrew Burgess Feb. 19, 2021, 5:57 p.m. | #2
* Andrew Burgess <andrew.burgess@embecosm.com> [2021-02-19 17:55:58 +0000]:

> * Tom Tromey <tromey@adacore.com> [2021-02-18 11:04:28 -0700]:

> 

> > This changes resolve_subexp to use any_of and the erase-remove idiom

> > to simplify the code somewhat.  This simplifies the next patch a bit.

> > 

> > gdb/ChangeLog

> > 2021-02-18  Tom Tromey  <tromey@adacore.com>

> > 

> > 	* ada-lang.c (resolve_subexp): Use any_of and erase-remove idiom.

> > ---

> >  gdb/ChangeLog  |  4 ++++

> >  gdb/ada-lang.c | 57 +++++++++++++++++++++++++-------------------------

> >  2 files changed, 32 insertions(+), 29 deletions(-)

> > 

> > diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c

> > index e2b2e6105b0..db4f3591131 100644

> > --- a/gdb/ada-lang.c

> > +++ b/gdb/ada-lang.c

> > @@ -3660,41 +3660,40 @@ resolve_subexp (expression_up *expp, int *pos, int deprocedure_p,

> >  	    ada_lookup_symbol_list (exp->elts[pc + 2].symbol->linkage_name (),

> >  				    exp->elts[pc + 1].block, VAR_DOMAIN,

> >  				    &candidates);

> > +	  /* Paranoia.  */

> > +	  candidates.resize (n_candidates);

> 

> It's not clear what you're being paranoid about here?  I would have

> expected that:

> 

>    gdb_assert (candidates.size () == n_candidates);

> 

> based on the comments on ada_lookup_symbol_list.  But even if that's

> not true I can't see what harm would occur if the size and

> n_candidates were out of sync.

> 

> Could you expand the command here to help those of us who are slower

> to catch on :)


OK, just looked at the next patch and saw that these get deleted
anyway, so don't waste your time.

This LGTM as is.

Thanks,
Andrew



> 

> Otherwise, I didn't try to confirm the code does exactly as before, I

> figure testing will do that, but the new code look very readable, so

> LGTM.

> 

> Thanks,

> Andrew

> 

> >  

> > -	  if (n_candidates > 1)

> > +	  if (std::any_of (candidates.begin (),

> > +			   candidates.end (),

> > +			   [] (block_symbol &sym)

> > +			   {

> > +			     switch (SYMBOL_CLASS (sym.symbol))

> > +			       {

> > +			       case LOC_REGISTER:

> > +			       case LOC_ARG:

> > +			       case LOC_REF_ARG:

> > +			       case LOC_REGPARM_ADDR:

> > +			       case LOC_LOCAL:

> > +			       case LOC_COMPUTED:

> > +				 return true;

> > +			       default:

> > +				 return false;

> > +			       }

> > +			   }))

> >  	    {

> >  	      /* Types tend to get re-introduced locally, so if there

> >  		 are any local symbols that are not types, first filter

> >  		 out all types.  */

> > -	      int j;

> > -	      for (j = 0; j < n_candidates; j += 1)

> > -		switch (SYMBOL_CLASS (candidates[j].symbol))

> > +	      candidates.erase

> > +		(std::remove_if

> > +		 (candidates.begin (),

> > +		  candidates.end (),

> > +		  [] (block_symbol &sym)

> >  		  {

> > -		  case LOC_REGISTER:

> > -		  case LOC_ARG:

> > -		  case LOC_REF_ARG:

> > -		  case LOC_REGPARM_ADDR:

> > -		  case LOC_LOCAL:

> > -		  case LOC_COMPUTED:

> > -		    goto FoundNonType;

> > -		  default:

> > -		    break;

> > -		  }

> > -	    FoundNonType:

> > -	      if (j < n_candidates)

> > -		{

> > -		  j = 0;

> > -		  while (j < n_candidates)

> > -		    {

> > -		      if (SYMBOL_CLASS (candidates[j].symbol) == LOC_TYPEDEF)

> > -			{

> > -			  candidates[j] = candidates[n_candidates - 1];

> > -			  n_candidates -= 1;

> > -			}

> > -		      else

> > -			j += 1;

> > -		    }

> > -		}

> > +		    return SYMBOL_CLASS (sym.symbol) == LOC_TYPEDEF;

> > +		  }),

> > +		 candidates.end ());

> > +	      n_candidates = candidates.size ();

> >  	    }

> >  

> >  	  if (n_candidates == 0)

> > -- 

> > 2.26.2

> >

Patch

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index e2b2e6105b0..db4f3591131 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -3660,41 +3660,40 @@  resolve_subexp (expression_up *expp, int *pos, int deprocedure_p,
 	    ada_lookup_symbol_list (exp->elts[pc + 2].symbol->linkage_name (),
 				    exp->elts[pc + 1].block, VAR_DOMAIN,
 				    &candidates);
+	  /* Paranoia.  */
+	  candidates.resize (n_candidates);
 
-	  if (n_candidates > 1)
+	  if (std::any_of (candidates.begin (),
+			   candidates.end (),
+			   [] (block_symbol &sym)
+			   {
+			     switch (SYMBOL_CLASS (sym.symbol))
+			       {
+			       case LOC_REGISTER:
+			       case LOC_ARG:
+			       case LOC_REF_ARG:
+			       case LOC_REGPARM_ADDR:
+			       case LOC_LOCAL:
+			       case LOC_COMPUTED:
+				 return true;
+			       default:
+				 return false;
+			       }
+			   }))
 	    {
 	      /* Types tend to get re-introduced locally, so if there
 		 are any local symbols that are not types, first filter
 		 out all types.  */
-	      int j;
-	      for (j = 0; j < n_candidates; j += 1)
-		switch (SYMBOL_CLASS (candidates[j].symbol))
+	      candidates.erase
+		(std::remove_if
+		 (candidates.begin (),
+		  candidates.end (),
+		  [] (block_symbol &sym)
 		  {
-		  case LOC_REGISTER:
-		  case LOC_ARG:
-		  case LOC_REF_ARG:
-		  case LOC_REGPARM_ADDR:
-		  case LOC_LOCAL:
-		  case LOC_COMPUTED:
-		    goto FoundNonType;
-		  default:
-		    break;
-		  }
-	    FoundNonType:
-	      if (j < n_candidates)
-		{
-		  j = 0;
-		  while (j < n_candidates)
-		    {
-		      if (SYMBOL_CLASS (candidates[j].symbol) == LOC_TYPEDEF)
-			{
-			  candidates[j] = candidates[n_candidates - 1];
-			  n_candidates -= 1;
-			}
-		      else
-			j += 1;
-		    }
-		}
+		    return SYMBOL_CLASS (sym.symbol) == LOC_TYPEDEF;
+		  }),
+		 candidates.end ());
+	      n_candidates = candidates.size ();
 	    }
 
 	  if (n_candidates == 0)