From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id D555B3858C66 for ; Thu, 12 Jan 2023 20:48:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D555B3858C66 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 30CKmFI7013692 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 12 Jan 2023 15:48:19 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 30CKmFI7013692 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1673556501; bh=hx85EisO7+FgMluEvS+L6WzsubpohO9Nw21MK70jDPk=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=VyozkNVuUBAEPSUs4oz+BXd9uABLBs2PSgy1tBQRxn3ck34DrmbfTb4k7AWSPuM8B YAGDknSQPSCCObmzWrF9xzw+AZW/FlV+PqHiQZUSyqocT7MDFJ8/bdWpPF5BWVP+OD 4dmT1SEvjir+hxLd5tRh0IKhUSft1qwS+IUUXYNU= Received: from [10.0.0.11] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id A44431E0D3; Thu, 12 Jan 2023 15:48:14 -0500 (EST) Message-ID: <368aabac-7418-3323-841a-45a16f82b796@polymtl.ca> Date: Thu, 12 Jan 2023 15:48:14 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH v8 4/6] GDB: Allow arbitrary keywords in integer set commands Content-Language: en-US To: "Maciej W. Rozycki" , gdb-patches@sourceware.org Cc: Andrew Burgess , Tom Tromey , Simon Sobisch References: From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 12 Jan 2023 20:48:15 +0000 X-Spam-Status: No, score=-3031.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_ASCII_DIVIDERS,NICE_REPLY_A,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 11/24/22 06:22, Maciej W. Rozycki wrote: > Rather than just `unlimited' allow the integer set commands (or command > options) to define arbitrary keywords for the user to use, removing > hardcoded arrangements for the `unlimited' keyword. > > Remove the confusingly named `var_zinteger', `var_zuinteger' and > `var_zuinteger_unlimited' `set'/`show' command variable types redefining > them in terms of `var_uinteger', `var_integer' and `var_pinteger', which > have the range of [0;UINT_MAX], [INT_MIN;INT_MAX], and [0;INT_MAX] each. > > Following existing practice `var_pinteger' allows extra negative values > to be used, however unlike `var_zuinteger_unlimited' any number of such > values can be defined rather than just `-1'. > > The "p" in `var_pinteger' stands for "positive", for the lack of a more > appropriate unambiguous letter, even though 0 obviously is not positive; > "n" would be confusing as to whether it stands for "non-negative" or > "negative". > > Add a new structure, `literal_def', the entries of which define extra > keywords allowed for a command and numerical values they correspond to. > Those values are not verified against the basic range supported by the > underlying variable type, allowing extra values to be allowed outside > that range, which may or may not be individually made visible to the > user. An optional value translation is possible with the structure to > follow the existing practice for some commands where user-entered 0 is > internally translated to UINT_MAX or INT_MAX. Such translation can now > be arbitrary. Literals defined by this structure are automatically used > for completion as necessary. > > So for example: > > const literal_def integer_unlimited_literals[] = > { > { "unlimited", INT_MAX, 0 }, > { nullptr } > }; > > defines an extra `unlimited' keyword and a user-visible 0 value, both of > which get translated to INT_MAX for the setting to be used with. > > Similarly: > > const literal_def zuinteger_unlimited_literals[] = > { > { "unlimited", -1, -1 }, > { nullptr } > }; > > defines the same keyword and a corresponding user-visible -1 value that > is used for the requested setting. If the last member were omitted (or > set to `{}') here, then only the keyword would be allowed for the user > to enter and while -1 would still be used internally trying to enter it > as a part of a command would result in an "integer -1 out of range" > error. > > Use said error message in all cases (citing the invalid value requested) > replacing "only -1 is allowed to set as unlimited" previously used for > `var_zuinteger_unlimited' settings only rather than propagating it to > `var_pinteger' type. It could only be used for the specific case where > a single extra `unlimited' keyword was defined standing for -1 and the > use of numeric equivalents is discouraged anyway as it is for historical > reasons only that they expose GDB internals, confusingly different > across variable types. Similarly update the "must be >= -1" Guile error > message. > > Redefine Guile and Python parameter types in terms of the new variable > types and interpret extra keywords as Scheme keywords and Python strings > used to communicate corresponding parameter values. Do not add a new > PARAM_INTEGER Guile parameter type, however do handle the `var_integer' > variable type now, permitting existing parameters defined by GDB proper, > such as `listsize', to be accessed from Scheme code. > > With these changes in place it should be trivial for a Scheme or Python > programmer to expand the syntax of the `make-parameter' command and the > `gdb.Parameter' class initializer to have arbitrary extra literals along > with their internal representation supplied. > > Update the testsuite accordingly. This is a nice cleanup I think. I didn't want to delay this further, so I did the best review I could given the time I had. I'm fine with you merging this (you can add my Approved-By), we can always follow up with more fixes. If others would like to review it, it's always appreciated, but given the time it's been sitting unreviewed, I kind of doubt that will happen. I noted a few minor things: > --- > Changes from v7: > > - Handle the special case of returning -1 for the value of `unlimited' > with `var_pinteger' parameters in Python code. > > - Update Python documentation accordingly, including `unlimited' case > descriptions from what used to be in 1/4. > > - Pass extra literals outside erased args with `add_set_or_show_cmd' and > up the call chain. > > No change from v6. > > Changes from v5: > > - Add a translation layer from Guile and Python parameter types to new > GDB variable types and remove `var_zuinteger', `var_uinteger', and > `var_zuinteger_unlimited' variable types altogether now. > > - Add an optional `extra_literals' initialiser to the `setting' class > constructor. > > - Remove the "only -1 is allowed to set as unlimited" error message > altogether now rather than propagating it to `var_pinteger' type. > > - Make the `val' member of `struct literal_def' optional and remove the > `allow' member; simplify processing accordingly. > > - Rename `zuinteger_unlimited_literals' to `pinteger_unlimited_literals', > making the names of all `*_unlimited_literals' arrays consistent with > the corresponding `var_*' variable types. > > - Rename `struct integer_option_def' to `struct pinteger_option_def', > observing it's come from `struct zuinteger_unlimited_option_def' and > what used to be the `var_zuinteger_unlimited' now has `var_pinteger' > semantics. > > - Update the names of test flags used by `maint test-options' accordingly. > > - Add constructor variants to `struct uinteger_option_def' and `struct > pinteger_option_def' that allow one to skip the `extra_literals' > initialiser altogether rather than having to pass in `nullptr'. > > - Update Python documentation mentioning the use of literal `unlimited' > with the respective parameter types. > > No change from v4. > > New change in v4. > --- > gdb/cli/cli-cmds.c | 59 ++--- > gdb/cli/cli-decode.c | 321 +++++++++++++++++++++++------- > gdb/cli/cli-option.c | 117 +++++++--- > gdb/cli/cli-option.h | 54 +++-- > gdb/cli/cli-setshow.c | 245 ++++++++++------------ > gdb/cli/cli-setshow.h | 20 - > gdb/command.h | 110 +++++++--- > gdb/doc/python.texi | 28 +- > gdb/guile/scm-param.c | 319 +++++++++++++++++++---------- > gdb/maint-test-options.c | 44 ++-- > gdb/python/py-param.c | 286 ++++++++++++++++---------- > gdb/python/python.c | 52 +++- > gdb/testsuite/gdb.base/max-value-size.exp | 2 > gdb/testsuite/gdb.base/options.exp | 47 ++-- > gdb/testsuite/gdb.base/settings.exp | 2 > gdb/testsuite/gdb.base/with.exp | 2 > gdb/testsuite/gdb.guile/scm-parameter.exp | 23 -- > gdb/testsuite/gdb.python/py-parameter.exp | 15 + > gdb/valprint.c | 9 > 19 files changed, 1133 insertions(+), 622 deletions(-) > > gdb-setshow-cmd-extra-literals.diff > Index: src/gdb/cli/cli-cmds.c > =================================================================== > --- src.orig/gdb/cli/cli-cmds.c > +++ src/gdb/cli/cli-cmds.c > @@ -2213,22 +2213,40 @@ value_from_setting (const setting &var, > { > switch (var.type ()) > { > + case var_uinteger: > case var_integer: > - if (var.get () == INT_MAX) > - return value_from_longest (builtin_type (gdbarch)->builtin_int, > - 0); > - else > - return value_from_longest (builtin_type (gdbarch)->builtin_int, > - var.get ()); > - case var_zinteger: > - return value_from_longest (builtin_type (gdbarch)->builtin_int, > - var.get ()); > + case var_pinteger: > + { > + LONGEST value > + = (var.type () == var_uinteger > + ? static_cast (var.get ()) > + : static_cast (var.get ())); > + > + if (var.extra_literals () != nullptr) > + for (const literal_def *l = var.extra_literals (); > + l->literal != nullptr; > + l++) > + if (value == l->use) > + { > + if (l->val.has_value ()) > + value = *l->val; > + else > + return allocate_value (builtin_type (gdbarch)->builtin_void); I was wondering about this line returning void. As far as I understand, this isn't used today. It would only be used if we defined a literal_def without a user-visible value. And the consequence would be that for this setting, using $_gdb_setting("foo") would yield void for that setting value. > + break; > + } > + > + if (var.type () == var_uinteger) > + return > + value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int, > + static_cast (value)); > + else > + return > + value_from_longest (builtin_type (gdbarch)->builtin_int, > + static_cast (value)); Unnecessary consts in casts. > Index: src/gdb/cli/cli-option.c > =================================================================== > --- src.orig/gdb/cli/cli-option.c > +++ src/gdb/cli/cli-option.c > @@ -38,7 +38,7 @@ union option_value > /* For var_uinteger options. */ > unsigned int uinteger; > > - /* For var_zuinteger_unlimited options. */ > + /* For var_integer and var_pinteger options. */ > int integer; > > /* For var_enum options. */ > @@ -356,42 +356,55 @@ parse_option (gdb::array_view return option_def_and_value {*match, match_ctx, val}; > } > case var_uinteger: > - case var_zuinteger_unlimited: > + case var_integer: > + case var_pinteger: > { > if (completion != nullptr) > { > - if (**args == '\0') > - { > - /* Convenience to let the user know what the option > - can accept. Note there's no common prefix between > - the strings on purpose, so that readline doesn't do > - a partial match. */ > - completion->tracker.add_completion > - (make_unique_xstrdup ("NUMBER")); > - completion->tracker.add_completion > - (make_unique_xstrdup ("unlimited")); > - return {}; > - } > - else if (startswith ("unlimited", *args)) > + if (match->extra_literals != nullptr) The two ifs could be merged, it would help reduce the indentation a bit. > { > - completion->tracker.add_completion > - (make_unique_xstrdup ("unlimited")); > - return {}; > + /* Convenience to let the user know what the option can > + accept. Make sure there's no common prefix between > + "NUMBER" and all the strings when adding new ones, > + so that readline doesn't do a partial match. */ > + if (**args == '\0') > + { > + completion->tracker.add_completion > + (make_unique_xstrdup ("NUMBER")); > + for (const literal_def *l = match->extra_literals; > + l->literal != nullptr; > + l++) > + completion->tracker.add_completion > + (make_unique_xstrdup (l->literal)); > + return {}; > + } > + else > + { > + bool completions = false; > + for (const literal_def *l = match->extra_literals; > + l->literal != nullptr; > + l++) > + if (startswith (l->literal, *args)) > + { > + completion->tracker.add_completion > + (make_unique_xstrdup (l->literal)); > + completions = true; > + } > + if (completions) > + return {}; > + } > } > } > > - if (match->type == var_zuinteger_unlimited) > - { > - option_value val; > - val.integer = parse_cli_var_zuinteger_unlimited (args, false); > - return option_def_and_value {*match, match_ctx, val}; > - } > + LONGEST v = parse_cli_var_integer (match->type, > + match->extra_literals, > + args, false); > + option_value val; > + if (match->type == var_uinteger) > + val.uinteger = v; > else > - { > - option_value val; > - val.uinteger = parse_cli_var_uinteger (match->type, args, false); > - return option_def_and_value {*match, match_ctx, val}; > - } > + val.integer = v; > + return option_def_and_value {*match, match_ctx, val}; > } > case var_enum: > { > @@ -593,7 +606,8 @@ save_option_value_in_ctx (gdb::optional< > *ov->option.var_address.uinteger (ov->option, ov->ctx) > = ov->value->uinteger; > break; > - case var_zuinteger_unlimited: > + case var_integer: > + case var_pinteger: > *ov->option.var_address.integer (ov->option, ov->ctx) > = ov->value->integer; > break; > @@ -664,8 +678,20 @@ get_val_type_str (const option_def &opt, > case var_boolean: > return "[on|off]"; > case var_uinteger: > - case var_zuinteger_unlimited: > - return "NUMBER|unlimited"; > + case var_integer: > + case var_pinteger: > + { > + buffer = "NUMBER"; > + if (opt.extra_literals != nullptr) > + for (const literal_def *l = opt.extra_literals; > + l->literal != nullptr; > + l++) > + { > + buffer += '|'; > + buffer += l->literal; > + } > + return buffer.c_str (); > + } > case var_enum: > { > buffer = ""; > @@ -789,20 +815,31 @@ add_setshow_cmds_for_options (command_cl > { > add_setshow_uinteger_cmd (option.name, cmd_class, > option.var_address.uinteger (option, data), > + option.extra_literals, > option.set_doc, option.show_doc, > option.help_doc, > nullptr, option.show_cmd_cb, > set_list, show_list); > } > - else if (option.type == var_zuinteger_unlimited) > + else if (option.type == var_integer) > { > - add_setshow_zuinteger_unlimited_cmd > - (option.name, cmd_class, > - option.var_address.integer (option, data), > - option.set_doc, option.show_doc, > - option.help_doc, > - nullptr, option.show_cmd_cb, > - set_list, show_list); > + add_setshow_integer_cmd (option.name, cmd_class, > + option.var_address.integer (option, data), > + option.extra_literals, > + option.set_doc, option.show_doc, > + option.help_doc, > + nullptr, option.show_cmd_cb, > + set_list, show_list); > + } > + else if (option.type == var_pinteger) > + { > + add_setshow_pinteger_cmd (option.name, cmd_class, > + option.var_address.integer (option, data), > + option.extra_literals, > + option.set_doc, option.show_doc, > + option.help_doc, > + nullptr, option.show_cmd_cb, > + set_list, show_list); > } > else if (option.type == var_enum) > { > Index: src/gdb/cli/cli-option.h > =================================================================== > --- src.orig/gdb/cli/cli-option.h > +++ src/gdb/cli/cli-option.h > @@ -49,12 +49,13 @@ struct option_def > used to create the option's "set/show" commands. */ > constexpr option_def (const char *name_, > var_types var_type_, > + const literal_def *extra_literals_, > 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_), > + : name (name_), type (var_type_), extra_literals (extra_literals_), > erased_get_var_address (erased_get_var_address_), > var_address {}, > show_cmd_cb (show_cmd_cb_), > @@ -68,6 +69,9 @@ struct option_def > /* The option's type. */ > var_types type; > > + /* Extra literals, such as `unlimited', accepted in lieu of a number. */ > + const literal_def *extra_literals; > + > /* A function that gets the controlling variable's address, type > erased. */ > erased_get_var_address_ftype *erased_get_var_address; > @@ -160,7 +164,7 @@ struct boolean_option_def : option_def > const char *set_doc_, > const char *show_doc_ = nullptr, > const char *help_doc_ = nullptr) > - : option_def (long_option_, var_boolean, > + : option_def (long_option_, var_boolean, nullptr, > (erased_get_var_address_ftype *) get_var_address_cb_, > show_cmd_cb_, > set_doc_, show_doc_, help_doc_) > @@ -207,37 +211,59 @@ struct uinteger_option_def : option_def > { > uinteger_option_def (const char *long_option_, > unsigned int *(*get_var_address_cb_) (Context *), > + const literal_def *extra_literals_, > 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, > + : option_def (long_option_, var_uinteger, extra_literals_, > (erased_get_var_address_ftype *) get_var_address_cb_, > show_cmd_cb_, > set_doc_, show_doc_, help_doc_) > { > var_address.uinteger = detail::get_var_address; > } > + > + uinteger_option_def (const char *long_option_, > + 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) > + : uinteger_option_def (long_option_, get_var_address_cb_, nullptr, > + show_cmd_cb_, set_doc_, show_doc_, help_doc_) > + { /* Nothing. */ } > }; > > -/* A var_zuinteger_unlimited command line option. */ > +/* A var_pinteger command line option. */ > > template > -struct zuinteger_unlimited_option_def : option_def > +struct pinteger_option_def : option_def > { > - zuinteger_unlimited_option_def (const char *long_option_, > - 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, > + pinteger_option_def (const char *long_option_, > + int *(*get_var_address_cb_) (Context *), > + const literal_def *extra_literals_, > + 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_pinteger, extra_literals_, > (erased_get_var_address_ftype *) get_var_address_cb_, > show_cmd_cb_, > set_doc_, show_doc_, help_doc_) > { > var_address.integer = detail::get_var_address; > } > + > + pinteger_option_def (const char *long_option_, > + 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) > + : pinteger_option_def (long_option_, get_var_address_cb_, nullptr, > + show_cmd_cb_, set_doc_, show_doc_, help_doc_) > + { /* Nothing. */ } > }; > > /* An var_enum command line option. */ > @@ -252,7 +278,7 @@ struct enum_option_def : option_def > const char *set_doc_, > const char *show_doc_ = nullptr, > const char *help_doc_ = nullptr) > - : option_def (long_option_, var_enum, > + : option_def (long_option_, var_enum, nullptr, > (erased_get_var_address_ftype *) get_var_address_cb_, > show_cmd_cb_, > set_doc_, show_doc_, help_doc_) > @@ -273,7 +299,7 @@ struct string_option_def : option_def > const char *set_doc_, > const char *show_doc_ = nullptr, > const char *help_doc_ = nullptr) > - : option_def (long_option_, var_string, > + : option_def (long_option_, var_string, nullptr, > (erased_get_var_address_ftype *) get_var_address_cb_, > show_cmd_cb_, > set_doc_, show_doc_, help_doc_) > Index: src/gdb/cli/cli-setshow.c > =================================================================== > --- src.orig/gdb/cli/cli-setshow.c > +++ src/gdb/cli/cli-setshow.c > @@ -149,10 +149,11 @@ deprecated_show_value_hack (struct ui_fi > } > } > > -/* Returns true if ARG is "unlimited". */ > +/* Returns true and the value in VAL if ARG is an accepted literal. */ > > static bool > -is_unlimited_literal (const char **arg, bool expression) > +get_literal_val (LONGEST &val, const literal_def *extra_literals, > + const char **arg, bool expression) > { > *arg = skip_spaces (*arg); > > @@ -162,85 +163,104 @@ is_unlimited_literal (const char **arg, > > size_t len = p - *arg; > > - if (len > 0 && strncmp ("unlimited", *arg, len) == 0) > - { > - *arg += len; > - > - /* If parsing an expression (i.e., parsing for a "set" command), > - anything after "unlimited" is junk. For options, anything > - after "unlimited" might be a command argument or another > - option. */ > - if (expression) > + if (len > 0 && extra_literals != nullptr) > + for (const literal_def *l = extra_literals; > + l->literal != nullptr; > + l++) > + if (strncmp (l->literal, *arg, len) == 0) > { > - const char *after = skip_spaces (*arg); > - if (*after != '\0') > - error (_("Junk after \"%.*s\": %s"), > - (int) len, unl_start, after); > - } > + *arg += len; > > - return true; > - } > + /* If parsing an expression (i.e., parsing for a "set" command), > + anything after the literal is junk. For options, anything > + after the literal might be a command argument or another > + option. */ > + if (expression) > + { > + const char *after = skip_spaces (*arg); > + if (*after != '\0') > + error (_("Junk after \"%.*s\": %s"), > + (int) len, unl_start, after); > + } > + > + val = l->use; > + return true; > + } > > return false; > } > > /* See cli-setshow.h. */ > > -unsigned int > -parse_cli_var_uinteger (var_types var_type, const char **arg, > - bool expression) > +LONGEST > +parse_cli_var_integer (var_types var_type, const literal_def *extra_literals, > + const char **arg, bool expression) The function here could perhaps take a `const setting &`, instead of var_type and extra_literals, which are properties of a setting. > @@ -623,36 +614,32 @@ get_setshow_command_value_string (const > } > break; > case var_uinteger: > - case var_zuinteger: > - { > - const unsigned int value = var.get (); > - > - if (var.type () == var_uinteger > - && value == UINT_MAX) > - stb.puts ("unlimited"); > - else > - stb.printf ("%u", value); > - } > - break; > case var_integer: > - case var_zinteger: > + case var_pinteger: > { > - const int value = var.get (); > + bool printed = false; > + const LONGEST value > + = (var.type () == var_uinteger > + ? static_cast (var.get ()) > + : static_cast (var.get ())); > > - if (var.type () == var_integer > - && value == INT_MAX) > - stb.puts ("unlimited"); > - else > - stb.printf ("%d", value); > - } > - break; > - case var_zuinteger_unlimited: > - { > - const int value = var.get (); > - if (value == -1) > - stb.puts ("unlimited"); > - else > - stb.printf ("%d", value); > + if (var.extra_literals () != nullptr) > + for (const literal_def *l = var.extra_literals (); > + l->literal != nullptr; > + l++) > + if (value == l->use) > + { > + stb.puts (l->literal); > + printed = true; > + break; > + } > + if (!printed) > + { > + if (var.type () == var_uinteger) > + stb.printf ("%u", static_cast (value)); > + else > + stb.printf ("%d", static_cast (value)); The consts are unnecessary here. > @@ -106,22 +108,25 @@ enum var_types > var_optional_filename, > /* String which stores a filename. (*VAR) is a std::string. */ > var_filename, > - /* ZeroableInteger. *VAR is an int. Like var_integer except > - that zero really means zero. */ > - var_zinteger, > - /* ZeroableUnsignedInteger. *VAR is an unsigned int. Zero really > - means zero. */ > - var_zuinteger, > - /* ZeroableUnsignedInteger with unlimited value. *VAR is an int, > - but its range is [0, INT_MAX]. -1 stands for unlimited and > - other negative numbers are not allowed. */ > - var_zuinteger_unlimited, > /* Enumerated type. Can only have one of the specified values. > *VAR is a char pointer to the name of the element that we > find. */ > var_enum > }; > > +/* A structure describing an extra literal accepted and shown in place > + of a number. */ > +struct literal_def > + { > + /* The literal to define, e.g. "unlimited". */ > + const char *literal; > + /* The number to substitute internally for LITERAL or VAL; > + the use of this number is not allowed (unless the same as VAL). */ > + LONGEST use; > + /* An optional number accepted that stands for the literal. */ > + gdb::optional val; Could you please add an empty line between each field? I find that makes it easier to read. > + }; The body of the struct should have one less indentation level (curly braces at column 0). > @@ -218,8 +222,8 @@ struct setting > > Type T must match the var type VAR_TYPE (see VAR_TYPE_USES). */ > template > - setting (var_types var_type, T *var) > - : m_var_type (var_type), m_var (var) > + setting (var_types var_type, T *var, const void *extra_literals = nullptr) > + : m_var_type (var_type), m_var (var), m_extra_literals (extra_literals) Any reason why extra_literals and m_extra_literals are void pointers, rather than literal_def pointers? > @@ -110,6 +116,50 @@ struct param_smob > SCM containing_scm; > }; > > +/* Guile parameter types as in PARAMETER_TYPES later on. */ > + > +typedef enum param_types > + { > + param_boolean, > + param_auto_boolean, > + param_zinteger, > + param_uinteger, > + param_zuinteger, > + param_zuinteger_unlimited, > + param_string, > + param_string_noescape, > + param_optional_filename, > + param_filename, > + param_enum, > + } > +param_types; Just "enum param_types, no need for the typedef. Braces at column 0. > + > +/* Translation from Guile parameters to GDB variable types. Keep in the > + same order as PARAM_TYPES due to C++'s lack of designated initializers. */ > + > +static const struct > + { > + /* The type of the parameter. */ > + enum var_types type; > + > + /* Extra literals, such as `unlimited', accepted in lieu of a number. */ > + const literal_def *extra_literals; > + } Braces at column 0 above. > @@ -631,20 +672,29 @@ pascm_param_value (const setting &var, i > return auto_keyword; > } > > - case var_zuinteger_unlimited: > - if (var.get () == -1) > - return unlimited_keyword; > - gdb_assert (var.get () >= 0); > - /* Fall through. */ > - case var_zinteger: > - return scm_from_int (var.get ()); > - > case var_uinteger: > - if (var.get ()== UINT_MAX) > - return unlimited_keyword; > - /* Fall through. */ > - case var_zuinteger: > - return scm_from_uint (var.get ()); > + case var_integer: > + case var_pinteger: > + { > + LONGEST value > + = (var.type () == var_uinteger > + ? static_cast (var.get ()) > + : static_cast (var.get ())); > + > + if (var.extra_literals () != nullptr) > + for (const literal_def *l = var.extra_literals (); > + l->literal != nullptr; > + l++) > + if (value == l->use) > + return scm_from_latin1_keyword (l->literal); > + if (var.type () == var_pinteger) > + gdb_assert (value >= 0); > + > + if (var.type () == var_uinteger) > + return scm_from_uint (static_cast (value)); > + else > + return scm_from_int (static_cast (value)); Unnecessary consts in casts. > + } > > default: > break; > @@ -735,53 +785,91 @@ pascm_set_param_value_x (param_smob *p_s > var.set (AUTO_BOOLEAN_FALSE); > break; > > - case var_zinteger: > + case var_integer: > case var_uinteger: > - case var_zuinteger: > - case var_zuinteger_unlimited: > - if (var.type () == var_uinteger > - || var.type () == var_zuinteger_unlimited) > - { > - SCM_ASSERT_TYPE (scm_is_integer (value) > - || scm_is_eq (value, unlimited_keyword), > - value, arg_pos, func_name, > - _("integer or #:unlimited")); > - if (scm_is_eq (value, unlimited_keyword)) > + case var_pinteger: > + { > + const literal_def *extra_literals = p_smob->extra_literals; > + enum tribool allowed = TRIBOOL_UNKNOWN; > + enum var_types var_type = var.type (); > + bool integer = scm_is_integer (value); > + bool keyword = scm_is_keyword (value); > + std::string buffer = ""; > + size_t count = 0; > + LONGEST val; > + > + if (extra_literals != nullptr) > + for (const literal_def *l = extra_literals; > + l->literal != nullptr; > + l++, count++) > { > - if (var.type () == var_uinteger) > - var.set (UINT_MAX); > - else > - var.set (-1); > - break; > + if (count != 0) > + buffer += ", "; > + buffer = buffer + "#:" + l->literal; > + if (keyword > + && allowed == TRIBOOL_UNKNOWN > + && scm_is_eq (value, > + scm_from_latin1_keyword (l->literal))) > + { > + val = l->use; > + allowed = TRIBOOL_TRUE; > + } > } > - } > - else > - { > - SCM_ASSERT_TYPE (scm_is_integer (value), value, arg_pos, func_name, > - _("integer")); > - } > > - if (var.type () == var_uinteger > - || var.type () == var_zuinteger) > - { > - unsigned int u = scm_to_uint (value); > + if (allowed == TRIBOOL_UNKNOWN) > + { > + if (extra_literals == nullptr) > + SCM_ASSERT_TYPE (integer, value, arg_pos, func_name, > + _("integer")); > + else if (count > 1) > + SCM_ASSERT_TYPE (integer, value, arg_pos, func_name, > + string_printf (_("integer or one of: %s"), > + buffer.c_str ()).c_str ()); > + else > + SCM_ASSERT_TYPE (integer, value, arg_pos, func_name, > + string_printf (_("integer or %s"), > + buffer.c_str ()).c_str ()); > > - if (var.type () == var_uinteger && u == 0) > - u = UINT_MAX; > - var.set (u); > - } > - else > - { > - int i = scm_to_int (value); > + val = (var_type == var_uinteger > + ? static_cast (scm_to_uint (value)) > + : static_cast (scm_to_int (value))); > > - if (var.type () == var_zuinteger_unlimited && i < -1) > - { > - gdbscm_out_of_range_error (func_name, arg_pos, value, > - _("must be >= -1")); > + if (extra_literals != nullptr) > + for (const literal_def *l = extra_literals; > + l->literal != nullptr; > + l++) > + { > + if (l->val.has_value () && val == *l->val) > + { > + allowed = TRIBOOL_TRUE; > + val = l->use; > + break; > + } > + else if (val == l->use) > + allowed = TRIBOOL_FALSE; > + } > } > - var.set (i); > - } > - break; > + > + if (allowed == TRIBOOL_UNKNOWN) > + { > + if (val > UINT_MAX || val < INT_MIN > + || (var_type == var_uinteger && val < 0) > + || (var_type == var_integer && val > INT_MAX) > + || (var_type == var_pinteger && val < 0) > + || (var_type == var_pinteger && val > INT_MAX)) > + allowed = TRIBOOL_FALSE; > + } > + if (allowed == TRIBOOL_FALSE) > + gdbscm_out_of_range_error (func_name, arg_pos, value, > + _("integer out of range")); > + > + if (var_type == var_uinteger) > + var.set (static_cast (val)); > + else > + var.set (static_cast (val)); Unnecessary consts in casts. > Index: src/gdb/python/py-param.c > =================================================================== > --- src.orig/gdb/python/py-param.c > +++ src/gdb/python/py-param.c > @@ -28,24 +28,70 @@ > #include "language.h" > #include "arch-utils.h" > > +/* Python parameter types as in PARM_CONSTANTS below. */ > + > +typedef enum param_types > + { > + param_boolean, > + param_auto_boolean, > + param_uinteger, > + param_integer, > + param_string, > + param_string_noescape, > + param_optional_filename, > + param_filename, > + param_zinteger, > + param_zuinteger, > + param_zuinteger_unlimited, > + param_enum, > + } > +param_types; Just "enum param_types, no need for the typedef. Braces at column 0. I think that having two `param_types` enums (here and in the Guile support) technically violates the one definition rule. Not sure if that matters in practice. > + > +/* Translation from Python parameters to GDB variable types. Keep in the > + same order as PARAM_TYPES due to C++'s lack of designated initializers. */ > + > +static const struct > + { > + /* The type of the parameter. */ > + enum var_types type; > + > + /* Extra literals, such as `unlimited', accepted in lieu of a number. */ > + const literal_def *extra_literals; > + } Braces at column 0. > Index: src/gdb/python/python.c > =================================================================== > --- src.orig/gdb/python/python.c > +++ src/gdb/python/python.c > @@ -504,27 +504,45 @@ gdbpy_parameter_value (const setting &va > Py_RETURN_NONE; > } > > - case var_integer: > - if (var.get () == INT_MAX) > - Py_RETURN_NONE; > - /* Fall through. */ > - case var_zinteger: > - case var_zuinteger_unlimited: > - return gdb_py_object_from_longest (var.get ()).release (); > - > case var_uinteger: > + case var_integer: > + case var_pinteger: > { > - unsigned int val = var.get (); > + LONGEST value > + = (var.type () == var_uinteger > + ? static_cast (var.get ()) > + : static_cast (var.get ())); > > - if (val == UINT_MAX) > - Py_RETURN_NONE; > - return gdb_py_object_from_ulongest (val).release (); > - } > + if (var.extra_literals () != nullptr) > + for (const literal_def *l = var.extra_literals (); > + l->literal != nullptr; > + l++) > + if (value == l->use) > + { > + if (strcmp (l->literal, "unlimited") == 0) > + { > + /* Compatibility hack for API brokennes. */ brokenness > + if (var.type () == var_pinteger > + && l->val.has_value () > + && *l->val == -1) > + value = -1; > + else > + Py_RETURN_NONE; > + } > + else if (l->val.has_value ()) > + value = *l->val; > + else > + return host_string_to_python_string (l->literal).release (); > + } > > - case var_zuinteger: > - { > - unsigned int val = var.get (); > - return gdb_py_object_from_ulongest (val).release (); > + if (var.type () == var_uinteger) > + return > + gdb_py_object_from_ulongest > + (static_cast (value)).release (); > + else > + return > + gdb_py_object_from_longest > + (static_cast (value)).release (); Unnecessary const in casts. Simon