[PATCH 0/2] ada_encode rewrites

[PATCH 0/2] ada_encode rewrites

[Tom Tromey]

I found a couple of minor things in ada_encode that I wanted to
change, and this short series is the result.

Tested on x8x-6 Fedora 32.

Tom

[PATCH 1/2] Constify ada_encode return type

[Tom Tromey]

Callers of ada_encode don't ever modify the result, and also don't own
it -- it is owned by a static buffer in ada_encode_1.  This patch
changes ada_encode to return a const char *, which I feel is somewhat
clearer.

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

	* ada-lang.h (ada_encode): Constify result.
	* ada-lang.c (ada_encode): Constify result.
	(ada_encode_1): Constify result.
---
 gdb/ChangeLog  | 6 ++++++
 gdb/ada-exp.y  | 6 +++---
 gdb/ada-lang.c | 4 ++--
 gdb/ada-lang.h | 2 +-
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y
index 57d89b01fec..61d8df9df48 100644
--- a/gdb/ada-exp.y
+++ b/gdb/ada-exp.y
@@ -1201,9 +1201,9 @@ write_var_or_type (struct parser_state *par_state,
   if (block == NULL)
     block = par_state->expression_context_block;
 
-  encoded_name = ada_encode (name0.ptr);
-  name_len = strlen (encoded_name);
-  encoded_name = obstack_strndup (&temp_parse_space, encoded_name, name_len);
+  const char *temp_name = ada_encode (name0.ptr);
+  name_len = strlen (temp_name);
+  encoded_name = obstack_strndup (&temp_parse_space, temp_name, name_len);
   for (depth = 0; depth < MAX_RENAMING_CHAIN_LENGTH; depth += 1)
     {
       int tail_index;
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 0df406bff48..2502d2847b2 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -923,7 +923,7 @@ const struct ada_opname_map ada_opname_table[] = {
    THROW_ERRORS, throw an error if invalid operator name is found.
    Otherwise, return NULL in that case.  */
 
-static char *
+static const char *
 ada_encode_1 (const char *decoded, bool throw_errors)
 {
   static char *encoding_buffer = NULL;
@@ -978,7 +978,7 @@ ada_encode_1 (const char *decoded, bool throw_errors)
 /* The "encoded" form of DECODED, according to GNAT conventions.
    The result is valid until the next call to ada_encode.  */
 
-char *
+const char *
 ada_encode (const char *decoded)
 {
   return ada_encode_1 (decoded, true);
diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
index ae313ce700a..949d16ce3a6 100644
--- a/gdb/ada-lang.h
+++ b/gdb/ada-lang.h
@@ -318,7 +318,7 @@ extern struct type *ada_get_base_type (struct type *);
 
 extern struct type *ada_check_typedef (struct type *);
 
-extern char *ada_encode (const char *);
+extern const char *ada_encode (const char *);
 
 extern const char *ada_enum_name (const char *);
 
-- 
2.26.2

Re: [PATCH 1/2] Constify ada_encode return type

[Simon Marchi]

On 2020-10-02 4:26 p.m., Tom Tromey wrote:
> Callers of ada_encode don't ever modify the result, and also don't own
> it -- it is owned by a static buffer in ada_encode_1.  This patch
> changes ada_encode to return a const char *, which I feel is somewhat
> clearer.
>
> gdb/ChangeLog
> 2020-10-02  Tom Tromey  <tromey@adacore.com>
>
> 	* ada-lang.h (ada_encode): Constify result.
> 	* ada-lang.c (ada_encode): Constify result.
> 	(ada_encode_1): Constify result.
> ---
>  gdb/ChangeLog  | 6 ++++++
>  gdb/ada-exp.y  | 6 +++---
>  gdb/ada-lang.c | 4 ++--
>  gdb/ada-lang.h | 2 +-
>  4 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y
> index 57d89b01fec..61d8df9df48 100644
> --- a/gdb/ada-exp.y
> +++ b/gdb/ada-exp.y
> @@ -1201,9 +1201,9 @@ write_var_or_type (struct parser_state *par_state,
>    if (block == NULL)
>      block = par_state->expression_context_block;
>
> -  encoded_name = ada_encode (name0.ptr);
> -  name_len = strlen (encoded_name);
> -  encoded_name = obstack_strndup (&temp_parse_space, encoded_name, name_len);
> +  const char *temp_name = ada_encode (name0.ptr);
> +  name_len = strlen (temp_name);
> +  encoded_name = obstack_strndup (&temp_parse_space, temp_name, name_len);

Could this be further simplified to:

  encoded_name = obstack_strdup (&temp_parse_space, ada_encode (name0.ptr));

?

In any case, this LGTM.

Simon

Re: [PATCH 1/2] Constify ada_encode return type

[Tom Tromey]

>> +  const char *temp_name = ada_encode (name0.ptr);
>> +  name_len = strlen (temp_name);
>> +  encoded_name = obstack_strndup (&temp_parse_space, temp_name, name_len);

Simon> Could this be further simplified to:
Simon>   encoded_name = obstack_strdup (&temp_parse_space, ada_encode (name0.ptr));
Simon> ?

name_len is used later in the function, so this would involve an extra
strlen.

Tom

[PATCH 2/2] Use obstack in ada_encode_1

[Tom Tromey]

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

Re: [PATCH 2/2] Use obstack in ada_encode_1

[Simon Marchi]

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

Re: [PATCH 2/2] Use obstack in ada_encode_1

[Tom Tromey]

>>>>> "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

Re: [PATCH 2/2] Use obstack in ada_encode_1

[Simon Marchi]

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

Re: [PATCH 2/2] Use obstack in ada_encode_1

[Joel Brobecker]

> 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