Re: [PATCH 12/24] Introduce generic command options framework

Pedro Alves
2019-05-25T10:31:35Z

On 5/25/19 8:43 AM, Philippe Waroquiers wrote:
> While quickly scanning the patch, I found a few typos ...

Thanks!

>
> On Wed, 2019-05-22 at 21:53 +0100, Pedro Alves wrote:
>> diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
>> new file mode 100644
>> index 00000000000..432555a953f
>> --- /dev/null
>> +++ b/gdb/cli/cli-option.c
> ...
>> +/* Helper for build_help. Appends an indended version of DOC into
>> + HELP. */
> indended -> indented
>> +
>> +static void
>> +append_indended_doc (const char *doc, std::string &help)
> Same in the function name
>

Fixed.

>
>> diff --git a/gdb/cli/cli-option.h b/gdb/cli/cli-option.h
>> new file mode 100644
>> index 00000000000..437fc09a61b
>> --- /dev/null
>> +++ b/gdb/cli/cli-option.h
> ...
>> +struct option_def
>> +{
>> + /* The ctor is protected because you're supposed to construct using
>> + one of bool_option_def, etc. below. */
>> +protected:
>> + typedef void *(erased_var_address_ftype) ();
>> +
>> + /* Construct an option. NAME_ is the option's name. VAR_TYPE_
>> + defines the option's type. ERASED_VAR_ADDRESS_ is a pointer to
>> + the option's control variable. SHOW_CMD_CB_ is a pointer to
> Isn't ERASED_VAR_ADDRESS_ the address of a function ?

You're right. I think it wasn't at some point... I've renamed it
and everything related to include "get_" in the name.

>
>
>> diff --git a/gdb/testsuite/gdb.base/options.exp b/gdb/testsuite/gdb.base/options.exp
>> new file mode 100644
>> index 00000000000..924d7aa544e
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/options.exp
> ...
>>
>> +# Miscelaneous tests.
> Miscellaneous
>

Fixed.

Here's the patch that I'm squashing in.

From 4bce43c41d08f88c5ae598d54adec9097dbdeab3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sat, 25 May 2019 11:24:51 +0100
Subject: [PATCH] philippe review

---
gdb/cli/cli-option.c | 8 ++++----
gdb/cli/cli-option.h | 36 ++++++++++++++++++------------------
gdb/testsuite/gdb.base/options.exp | 2 +-
3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
index 432555a953f..9793dd5727e 100644
--- a/gdb/cli/cli-option.c
+++ b/gdb/cli/cli-option.c
@@ -601,11 +601,11 @@ get_val_type_str (const option_def &opt, std::string &buffer)
}
}

-/* Helper for build_help. Appends an indended version of DOC into
+/* Helper for build_help. Appends an indented version of DOC into
HELP. */

static void
-append_indended_doc (const char *doc, std::string &help)
+append_indented_doc (const char *doc, std::string &help)
{
const char *p = doc;
const char *n = strchr (p, '\n');
@@ -646,9 +646,9 @@ build_help_option (gdb::array_view<const option_def> options,
help += val_type_str;
}
help += "\n";
- append_indended_doc (o.set_doc, help);
+ append_indented_doc (o.set_doc, help);
if (o.help_doc != nullptr)
- append_indended_doc (o.help_doc, help);
+ append_indented_doc (o.help_doc, help);
help += '\n';
}
}
diff --git a/gdb/cli/cli-option.h b/gdb/cli/cli-option.h
index 437fc09a61b..1bfbfce1ce5 100644
--- a/gdb/cli/cli-option.h
+++ b/gdb/cli/cli-option.h
@@ -39,23 +39,23 @@ struct option_def
/* The ctor is protected because you're supposed to construct using
one of bool_option_def, etc. below. */
protected:
- typedef void *(erased_var_address_ftype) ();
+ typedef void *(erased_get_var_address_ftype) ();

/* Construct an option. NAME_ is the option's name. VAR_TYPE_
- defines the option's type. ERASED_VAR_ADDRESS_ is a pointer to
- the option's control variable. SHOW_CMD_CB_ is a pointer to
- callback for the "show" command that is installed for this
- option. SET_DOC_, SHOW_DOC_, HELP_DOC_ are used to create the
- option's "set/show" commands. */
+ defines the option's type. ERASED_GET_VAR_ADDRESS_ is a pointer
+ to a function that returns the option's control variable.
+ SHOW_CMD_CB_ is a pointer to callback for the "show" command that
+ is installed for this option. SET_DOC_, SHOW_DOC_, HELP_DOC_ are
+ used to create the option's "set/show" commands. */
constexpr option_def (const char *name_,
var_types var_type_,
- erased_var_address_ftype *erased_var_address_,
+ erased_get_var_address_ftype *erased_get_var_address_,
show_value_ftype *show_cmd_cb_,
const char *set_doc_,
const char *show_doc_,
const char *help_doc_)
: name (name_), type (var_type_),
- erased_var_address (erased_var_address_),
+ erased_get_var_address (erased_get_var_address_),
var_address {},
show_cmd_cb (show_cmd_cb_),
set_doc (set_doc_), show_doc (show_doc_), help_doc (help_doc_)
@@ -70,7 +70,7 @@ public:

/* A function that gets the controlling variable's address, type
erased. */
- erased_var_address_ftype *erased_var_address;
+ erased_get_var_address_ftype *erased_get_var_address;

/* Get the controlling variable's address. Each type of variable
uses a different union member. We do this instead of having a
@@ -128,7 +128,7 @@ static inline RetType *
get_var_address (const option_def &option, void *ctx)
{
using unerased_ftype = RetType *(Context *);
- unerased_ftype *fun = (unerased_ftype *) option.erased_var_address;
+ unerased_ftype *fun = (unerased_ftype *) option.erased_get_var_address;
return fun ((Context *) ctx);
}

@@ -154,13 +154,13 @@ template<typename Context>
struct boolean_option_def : option_def
{
boolean_option_def (const char *long_option_,
- int *(*var_address_cb_) (Context *),
+ int *(*get_var_address_cb_) (Context *),
show_value_ftype *show_cmd_cb_,
const char *set_doc_,
const char *show_doc_ = nullptr,
const char *help_doc_ = nullptr)
: option_def (long_option_, var_boolean,
- (erased_var_address_ftype *) var_address_cb_,
+ (erased_get_var_address_ftype *) get_var_address_cb_,
show_cmd_cb_,
set_doc_, show_doc_, help_doc_)
{
@@ -205,13 +205,13 @@ template<typename Context>
struct uinteger_option_def : option_def
{
uinteger_option_def (const char *long_option_,
- unsigned int *(*var_address_cb_) (Context *),
+ unsigned int *(*get_var_address_cb_) (Context *),
show_value_ftype *show_cmd_cb_,
const char *set_doc_,
const char *show_doc_ = nullptr,
const char *help_doc_ = nullptr)
: option_def (long_option_, var_uinteger,
- (erased_var_address_ftype *) var_address_cb_,
+ (erased_get_var_address_ftype *) get_var_address_cb_,
show_cmd_cb_,
set_doc_, show_doc_, help_doc_)
{
@@ -225,13 +225,13 @@ template<typename Context>
struct zuinteger_unlimited_option_def : option_def
{
zuinteger_unlimited_option_def (const char *long_option_,
- int *(*var_address_cb_) (Context *),
+ int *(*get_var_address_cb_) (Context *),
show_value_ftype *show_cmd_cb_,
const char *set_doc_,
const char *show_doc_ = nullptr,
const char *help_doc_ = nullptr)
: option_def (long_option_, var_zuinteger_unlimited,
- (erased_var_address_ftype *) var_address_cb_,
+ (erased_get_var_address_ftype *) get_var_address_cb_,
show_cmd_cb_,
set_doc_, show_doc_, help_doc_)
{
@@ -246,13 +246,13 @@ struct enum_option_def : option_def
{
enum_option_def (const char *long_option_,
const char *const *enumlist,
- const char **(*var_address_cb_) (Context *),
+ const char **(*get_var_address_cb_) (Context *),
show_value_ftype *show_cmd_cb_,
const char *set_doc_,
const char *show_doc_ = nullptr,
const char *help_doc_ = nullptr)
: option_def (long_option_, var_enum,
- (erased_var_address_ftype *) var_address_cb_,
+ (erased_get_var_address_ftype *) get_var_address_cb_,
show_cmd_cb_,
set_doc_, show_doc_, help_doc_)
{
diff --git a/gdb/testsuite/gdb.base/options.exp b/gdb/testsuite/gdb.base/options.exp
index 924d7aa544e..1891176dc1d 100644
--- a/gdb/testsuite/gdb.base/options.exp
+++ b/gdb/testsuite/gdb.base/options.exp
@@ -117,7 +117,7 @@ set all_options {
"-zuinteger-unlimited"
}

-# Miscelaneous tests.
+# Miscellaneous tests.
proc_with_prefix test-misc {variant} {
global all_options

--
2.14.5