[RFA,v2,10/24] Remove make_cleanup_restore_current_language
Commit Message
This patch replaces make_cleanup_restore_current_language with an RAII
class that saves the current language, and restores it when the object
is destroyed.
ChangeLog
2017-07-25 Tom Tromey <tom@tromey.com>
* utils.h (make_cleanup_restore_current_language): Remove.
* utils.c (do_restore_current_language)
(make_cleanup_restore_current_language): Remove.
* parse.c (parse_exp_in_context_1)
(parse_expression_with_language): Use scoped_restore_language.
* mi/mi-main.c (mi_cmd_execute): Use scoped_restore_language.
* language.h (scoped_restore_language): New class.
---
gdb/ChangeLog | 10 ++++++++++
gdb/language.h | 26 ++++++++++++++++++++++++++
gdb/mi/mi-main.c | 6 ++----
gdb/parse.c | 22 +++++++---------------
gdb/utils.c | 22 ----------------------
gdb/utils.h | 2 --
6 files changed, 45 insertions(+), 43 deletions(-)
Comments
Hi Tom,
On 2017-07-25 07:20 PM, Tom Tromey wrote:
> This patch replaces make_cleanup_restore_current_language with an RAII
> class that saves the current language, and restores it when the object
> is destroyed.
>
> ChangeLog
> 2017-07-25 Tom Tromey <tom@tromey.com>
>
> * utils.h (make_cleanup_restore_current_language): Remove.
> * utils.c (do_restore_current_language)
> (make_cleanup_restore_current_language): Remove.
> * parse.c (parse_exp_in_context_1)
> (parse_expression_with_language): Use scoped_restore_language.
> * mi/mi-main.c (mi_cmd_execute): Use scoped_restore_language.
> * language.h (scoped_restore_language): New class.
> ---
> gdb/ChangeLog | 10 ++++++++++
> gdb/language.h | 26 ++++++++++++++++++++++++++
> gdb/mi/mi-main.c | 6 ++----
> gdb/parse.c | 22 +++++++---------------
> gdb/utils.c | 22 ----------------------
> gdb/utils.h | 2 --
> 6 files changed, 45 insertions(+), 43 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 871b1f0..37971ec 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,15 @@
> 2017-07-25 Tom Tromey <tom@tromey.com>
>
> + * utils.h (make_cleanup_restore_current_language): Remove.
> + * utils.c (do_restore_current_language)
> + (make_cleanup_restore_current_language): Remove.
> + * parse.c (parse_exp_in_context_1)
> + (parse_expression_with_language): Use scoped_restore_language.
> + * mi/mi-main.c (mi_cmd_execute): Use scoped_restore_language.
> + * language.h (scoped_restore_language): New class.
> +
> +2017-07-25 Tom Tromey <tom@tromey.com>
> +
> * source.c (get_filename_and_charpos, forward_search_command)
> (reverse_search_command): Use fd_closer.
> * procfs.c (load_syscalls, proc_get_LDT_entry)
> diff --git a/gdb/language.h b/gdb/language.h
> index f4852c1..3586526 100644
> --- a/gdb/language.h
> +++ b/gdb/language.h
> @@ -633,4 +633,30 @@ extern const struct language_defn opencl_language_defn;
> extern const struct language_defn pascal_language_defn;
> extern const struct language_defn rust_language_defn;
>
> +/* Save the current language and restore it upon destruction. */
> +
> +class scoped_restore_language
> +{
> +public:
> +
> + explicit scoped_restore_language (enum language new_lang)
> + : m_lang (current_language->la_language)
> + {
> + set_language (new_lang);
> + }
To follow the nomenclature and behavior of other scoped_restore_* , I think this:
1. should be named scoped_restore_current_language
2. should only only save and restore the current language, and not set the new language.
About #2, I think it's clearer to have:
scoped_restore_current_language ();
set_language (some_language);
than
scope_restore_current_language (some_language);
The 2nd leads me to think that the current language will be restored to some_language.
Thanks,
Simon
>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
>> + explicit scoped_restore_language (enum language new_lang)
>> + : m_lang (current_language->la_language)
>> + {
>> + set_language (new_lang);
>> + }
Simon> To follow the nomenclature and behavior of other scoped_restore_*
Simon> , I think this:
Simon> 1. should be named scoped_restore_current_language
Ok.
Simon> 2. should only only save and restore the current language, and
Simon> not set the new language.
scoped_restore allows setting the new value. That's what I was copying
here. From scoped_restore.h:
/* Create a new scoped_restore object that saves the current value
of *VAR, and sets *VAR to VALUE. *VAR will be restored when this
scoped_restore object is destroyed. This is templated on T2 to
allow passing VALUEs of types convertible to T.
E.g.: T='base'; T2='derived'. */
template <typename T2>
scoped_restore_tmpl (T *var, T2 value)
: scoped_restore_base (var),
m_saved_value (*var)
{
*var = value;
}
Tom
On 2017-08-01 00:17, Tom Tromey wrote:
> Simon> 2. should only only save and restore the current language, and
> Simon> not set the new language.
>
> scoped_restore allows setting the new value. That's what I was copying
> here. From scoped_restore.h:
>
> /* Create a new scoped_restore object that saves the current value
> of *VAR, and sets *VAR to VALUE. *VAR will be restored when this
> scoped_restore object is destroyed. This is templated on T2 to
> allow passing VALUEs of types convertible to T.
> E.g.: T='base'; T2='derived'. */
> template <typename T2>
> scoped_restore_tmpl (T *var, T2 value)
> : scoped_restore_base (var),
> m_saved_value (*var)
> {
> *var = value;
> }
>
> Tom
I'd argue the same thing for scoped_restore, but if I'm the only one
bothered by it, I can live with it.
Simon
@@ -1,5 +1,15 @@
2017-07-25 Tom Tromey <tom@tromey.com>
+ * utils.h (make_cleanup_restore_current_language): Remove.
+ * utils.c (do_restore_current_language)
+ (make_cleanup_restore_current_language): Remove.
+ * parse.c (parse_exp_in_context_1)
+ (parse_expression_with_language): Use scoped_restore_language.
+ * mi/mi-main.c (mi_cmd_execute): Use scoped_restore_language.
+ * language.h (scoped_restore_language): New class.
+
+2017-07-25 Tom Tromey <tom@tromey.com>
+
* source.c (get_filename_and_charpos, forward_search_command)
(reverse_search_command): Use fd_closer.
* procfs.c (load_syscalls, proc_get_LDT_entry)
@@ -633,4 +633,30 @@ extern const struct language_defn opencl_language_defn;
extern const struct language_defn pascal_language_defn;
extern const struct language_defn rust_language_defn;
+/* Save the current language and restore it upon destruction. */
+
+class scoped_restore_language
+{
+public:
+
+ explicit scoped_restore_language (enum language new_lang)
+ : m_lang (current_language->la_language)
+ {
+ set_language (new_lang);
+ }
+
+ ~scoped_restore_language ()
+ {
+ set_language (m_lang);
+ }
+
+ scoped_restore_language (const scoped_restore_language &) = delete;
+ scoped_restore_language &operator= (const scoped_restore_language &)
+ = delete;
+
+private:
+
+ enum language m_lang;
+};
+
#endif /* defined (LANGUAGE_H) */
@@ -2243,11 +2243,9 @@ mi_cmd_execute (struct mi_parse *parse)
error (_("Invalid frame id: %d"), frame);
}
+ gdb::optional<scoped_restore_language> lang_saver;
if (parse->language != language_unknown)
- {
- make_cleanup_restore_current_language ();
- set_language (parse->language);
- }
+ lang_saver.emplace (parse->language);
current_context = parse;
@@ -50,6 +50,7 @@
#include "objfiles.h"
#include "user-regs.h"
#include <algorithm>
+#include "common/gdb_optional.h"
/* Standard set of definitions for printing, dumping, prefixifying,
* and evaluating expressions. */
@@ -1136,7 +1137,7 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
const struct block *block,
int comma, int void_context_p, int *out_subexp)
{
- struct cleanup *old_chain, *inner_chain;
+ struct cleanup *old_chain;
const struct language_defn *lang = NULL;
struct parser_state ps;
int subexp;
@@ -1214,8 +1215,8 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
to the value matching SELECTED_FRAME as set by get_current_arch. */
initialize_expout (&ps, 10, lang, get_current_arch ());
- inner_chain = make_cleanup_restore_current_language ();
- set_language (lang->la_language);
+
+ scoped_restore_language lang_saver (lang->la_language);
TRY
{
@@ -1250,7 +1251,6 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
if (expressiondebug)
dump_prefix_expression (ps.expout, gdb_stdlog);
- do_cleanups (inner_chain);
discard_cleanups (old_chain);
*stringptr = lexptr;
@@ -1275,19 +1275,11 @@ parse_expression (const char *string)
expression_up
parse_expression_with_language (const char *string, enum language lang)
{
- struct cleanup *old_chain = NULL;
-
+ gdb::optional<scoped_restore_language> lang_saver;
if (current_language->la_language != lang)
- {
- old_chain = make_cleanup_restore_current_language ();
- set_language (lang);
- }
-
- expression_up expr = parse_expression (string);
+ lang_saver.emplace (lang);
- if (old_chain != NULL)
- do_cleanups (old_chain);
- return expr;
+ return parse_expression (string);
}
/* Parse STRING as an expression. If parsing ends in the middle of a
@@ -286,28 +286,6 @@ make_cleanup_free_so (struct so_list *so)
return make_cleanup (do_free_so, so);
}
-/* Helper for make_cleanup_restore_current_language. */
-
-static void
-do_restore_current_language (void *p)
-{
- enum language saved_lang = (enum language) (uintptr_t) p;
-
- set_language (saved_lang);
-}
-
-/* Remember the current value of CURRENT_LANGUAGE and make it restored when
- the cleanup is run. */
-
-struct cleanup *
-make_cleanup_restore_current_language (void)
-{
- enum language saved_lang = current_language->la_language;
-
- return make_cleanup (do_restore_current_language,
- (void *) (uintptr_t) saved_lang);
-}
-
/* Helper function for make_cleanup_clear_parser_state. */
static void
@@ -113,8 +113,6 @@ extern struct cleanup *make_cleanup_value_free (struct value *);
struct so_list;
extern struct cleanup *make_cleanup_free_so (struct so_list *so);
-extern struct cleanup *make_cleanup_restore_current_language (void);
-
/* A deleter for a hash table. */
struct htab_deleter
{