* [PATCH 0/2] ada_encode rewrites
@ 2020-10-02 20:26 Tom Tromey
2020-10-02 20:26 ` [PATCH 1/2] Constify ada_encode return type Tom Tromey
2020-10-02 20:26 ` [PATCH 2/2] Use obstack in ada_encode_1 Tom Tromey
0 siblings, 2 replies; 9+ messages in thread
From: Tom Tromey @ 2020-10-02 20:26 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] Constify ada_encode return type
2020-10-02 20:26 [PATCH 0/2] ada_encode rewrites Tom Tromey
@ 2020-10-02 20:26 ` Tom Tromey
2020-10-02 21:51 ` Simon Marchi
2020-10-02 20:26 ` [PATCH 2/2] Use obstack in ada_encode_1 Tom Tromey
1 sibling, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2020-10-02 20:26 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] Use obstack in ada_encode_1
2020-10-02 20:26 [PATCH 0/2] ada_encode rewrites Tom Tromey
2020-10-02 20:26 ` [PATCH 1/2] Constify ada_encode return type Tom Tromey
@ 2020-10-02 20:26 ` Tom Tromey
2020-10-02 21:54 ` Simon Marchi
1 sibling, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2020-10-02 20:26 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Constify ada_encode return type
2020-10-02 20:26 ` [PATCH 1/2] Constify ada_encode return type Tom Tromey
@ 2020-10-02 21:51 ` Simon Marchi
2020-10-07 18:26 ` Tom Tromey
0 siblings, 1 reply; 9+ messages in thread
From: Simon Marchi @ 2020-10-02 21:51 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Use obstack in ada_encode_1
2020-10-02 20:26 ` [PATCH 2/2] Use obstack in ada_encode_1 Tom Tromey
@ 2020-10-02 21:54 ` Simon Marchi
2020-10-07 18:35 ` Tom Tromey
0 siblings, 1 reply; 9+ messages in thread
From: Simon Marchi @ 2020-10-02 21:54 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Constify ada_encode return type
2020-10-02 21:51 ` Simon Marchi
@ 2020-10-07 18:26 ` Tom Tromey
0 siblings, 0 replies; 9+ messages in thread
From: Tom Tromey @ 2020-10-07 18:26 UTC (permalink / raw)
To: Simon Marchi; +Cc: Tom Tromey, gdb-patches
>> + 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Use obstack in ada_encode_1
2020-10-02 21:54 ` Simon Marchi
@ 2020-10-07 18:35 ` Tom Tromey
2020-10-07 18:36 ` Simon Marchi
2020-10-09 20:14 ` Joel Brobecker
0 siblings, 2 replies; 9+ messages in thread
From: Tom Tromey @ 2020-10-07 18:35 UTC (permalink / raw)
To: Simon Marchi; +Cc: Tom Tromey, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Use obstack in ada_encode_1
2020-10-07 18:35 ` Tom Tromey
@ 2020-10-07 18:36 ` Simon Marchi
2020-10-09 20:14 ` Joel Brobecker
1 sibling, 0 replies; 9+ messages in thread
From: Simon Marchi @ 2020-10-07 18:36 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Use obstack in ada_encode_1
2020-10-07 18:35 ` Tom Tromey
2020-10-07 18:36 ` Simon Marchi
@ 2020-10-09 20:14 ` Joel Brobecker
1 sibling, 0 replies; 9+ messages in thread
From: Joel Brobecker @ 2020-10-09 20:14 UTC (permalink / raw)
To: Tom Tromey; +Cc: Simon Marchi, gdb-patches
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-09 20:14 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-02 20:26 [PATCH 0/2] ada_encode rewrites Tom Tromey
2020-10-02 20:26 ` [PATCH 1/2] Constify ada_encode return type Tom Tromey
2020-10-02 21:51 ` Simon Marchi
2020-10-07 18:26 ` Tom Tromey
2020-10-02 20:26 ` [PATCH 2/2] Use obstack in ada_encode_1 Tom Tromey
2020-10-02 21:54 ` Simon Marchi
2020-10-07 18:35 ` Tom Tromey
2020-10-07 18:36 ` Simon Marchi
2020-10-09 20:14 ` Joel Brobecker
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).