* [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
* 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 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
* [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 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 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).