[2/2] Use obstack in ada_encode_1

Message ID 20201002202604.1517475-3-tromey@adacore.com
State New
Headers show
Series
  • ada_encode rewrites
Related show

Commit Message

Tom Tromey Oct. 2, 2020, 8:26 p.m.
I'd eventually like to get rid of the GROW_VECT macro; and since I had
already touched ada_encode_1 I decided to start here.  This patch
removes the use of the macro in favor of using an obstack in this
function.

2020-10-02  Tom Tromey  <tromey@adacore.com>

	* ada-lang.c (ada_encode_1): Use an obstack.
---
 gdb/ChangeLog  |  4 ++++
 gdb/ada-lang.c | 26 +++++++-------------------
 2 files changed, 11 insertions(+), 19 deletions(-)

-- 
2.26.2

Comments

Simon Marchi Oct. 2, 2020, 9:54 p.m. | #1
On 2020-10-02 4:26 p.m., Tom Tromey wrote:
> I'd eventually like to get rid of the GROW_VECT macro; and since I had

> already touched ada_encode_1 I decided to start here.  This patch

> removes the use of the macro in favor of using an obstack in this

> function.

>

> 2020-10-02  Tom Tromey  <tromey@adacore.com>

>

> 	* ada-lang.c (ada_encode_1): Use an obstack.

> ---

>  gdb/ChangeLog  |  4 ++++

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

>  2 files changed, 11 insertions(+), 19 deletions(-)

>

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

> index 2502d2847b2..dfe9d44959f 100644

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

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

> @@ -926,25 +926,17 @@ const struct ada_opname_map ada_opname_table[] = {

>  static const char *

>  ada_encode_1 (const char *decoded, bool throw_errors)

>  {

> -  static char *encoding_buffer = NULL;

> -  static size_t encoding_buffer_size = 0;

> +  static auto_obstack encoding_buffer;

>    const char *p;

> -  int k;

>

>    if (decoded == NULL)

>      return NULL;

>

> -  GROW_VECT (encoding_buffer, encoding_buffer_size,

> -             2 * strlen (decoded) + 10);

> -

> -  k = 0;

> +  encoding_buffer.clear ();

>    for (p = decoded; *p != '\0'; p += 1)

>      {

>        if (*p == '.')

> -        {

> -          encoding_buffer[k] = encoding_buffer[k + 1] = '_';

> -          k += 2;

> -        }

> +	obstack_grow_str (&encoding_buffer, "__");

>        else if (*p == '"')

>          {

>            const struct ada_opname_map *mapping;

> @@ -960,19 +952,15 @@ ada_encode_1 (const char *decoded, bool throw_errors)

>  	      else

>  		return NULL;

>  	    }

> -          strcpy (encoding_buffer + k, mapping->encoded);

> -          k += strlen (mapping->encoded);

> +	  obstack_grow_str (&encoding_buffer, mapping->encoded);

>            break;

>          }

>        else

> -        {

> -          encoding_buffer[k] = *p;

> -          k += 1;

> -        }

> +	obstack_1grow (&encoding_buffer, *p);

>      }

>

> -  encoding_buffer[k] = '\0';

> -  return encoding_buffer;

> +  obstack_1grow (&encoding_buffer, '\0');

> +  return (const char *) obstack_finish (&encoding_buffer);

>  }

>

>  /* The "encoded" form of DECODED, according to GNAT conventions.

> --

> 2.26.2

>


I think the same could be achieved using a static std::string instead of
an obstack, and it would be a bit more C++-y, but either way is fine
with me.

Simon
Tom Tromey Oct. 7, 2020, 6:35 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> I think the same could be achieved using a static std::string instead of
Simon> an obstack, and it would be a bit more C++-y, but either way is fine
Simon> with me.

Yeah, actually it is probably a better API to simply return a new
string.  I avoided this since I wasn't sure about why the function is
the way it is -- is it to avoid the difficulties of managing an
allocation in C, or is it to avoid extra allocations?  But I looked at
the callers and I think it will be fine.

Tom
Simon Marchi Oct. 7, 2020, 6:36 p.m. | #3
On 2020-10-07 2:35 p.m., Tom Tromey wrote:
> Yeah, actually it is probably a better API to simply return a new

> string.  I avoided this since I wasn't sure about why the function is

> the way it is -- is it to avoid the difficulties of managing an

> allocation in C, or is it to avoid extra allocations?  But I looked at

> the callers and I think it will be fine.


I always wonder the same with these functions and I have no idea.

Simon
Joel Brobecker Oct. 9, 2020, 8:14 p.m. | #4
> Yeah, actually it is probably a better API to simply return a new

> string.  I avoided this since I wasn't sure about why the function is

> the way it is -- is it to avoid the difficulties of managing an

> allocation in C, or is it to avoid extra allocations?  But I looked at

> the callers and I think it will be fine.


FWIW, a lot of it was before I started working on GDB, but my
understanding was that this was really meant to help managing
allocation lifetime, which as we know is tricky in C. If std::string
makes sense, I would indeed use that. I don't think this area is
a hot-spot during performance-sensitive operations.

-- 
Joel

Patch

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 2502d2847b2..dfe9d44959f 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -926,25 +926,17 @@  const struct ada_opname_map ada_opname_table[] = {
 static const char *
 ada_encode_1 (const char *decoded, bool throw_errors)
 {
-  static char *encoding_buffer = NULL;
-  static size_t encoding_buffer_size = 0;
+  static auto_obstack encoding_buffer;
   const char *p;
-  int k;
 
   if (decoded == NULL)
     return NULL;
 
-  GROW_VECT (encoding_buffer, encoding_buffer_size,
-             2 * strlen (decoded) + 10);
-
-  k = 0;
+  encoding_buffer.clear ();
   for (p = decoded; *p != '\0'; p += 1)
     {
       if (*p == '.')
-        {
-          encoding_buffer[k] = encoding_buffer[k + 1] = '_';
-          k += 2;
-        }
+	obstack_grow_str (&encoding_buffer, "__");
       else if (*p == '"')
         {
           const struct ada_opname_map *mapping;
@@ -960,19 +952,15 @@  ada_encode_1 (const char *decoded, bool throw_errors)
 	      else
 		return NULL;
 	    }
-          strcpy (encoding_buffer + k, mapping->encoded);
-          k += strlen (mapping->encoded);
+	  obstack_grow_str (&encoding_buffer, mapping->encoded);
           break;
         }
       else
-        {
-          encoding_buffer[k] = *p;
-          k += 1;
-        }
+	obstack_1grow (&encoding_buffer, *p);
     }
 
-  encoding_buffer[k] = '\0';
-  return encoding_buffer;
+  obstack_1grow (&encoding_buffer, '\0');
+  return (const char *) obstack_finish (&encoding_buffer);
 }
 
 /* The "encoded" form of DECODED, according to GNAT conventions.