From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x129.google.com (mail-lf1-x129.google.com [IPv6:2a00:1450:4864:20::129]) by sourceware.org (Postfix) with ESMTPS id 02BAA3858CDA for ; Thu, 19 Jan 2023 21:17:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 02BAA3858CDA Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=embecosm.com Received: by mail-lf1-x129.google.com with SMTP id x40so5075990lfu.12 for ; Thu, 19 Jan 2023 13:17:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=mime-version:user-agent:references:message-id:in-reply-to:subject :cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=cbrmFiFJ7inYSjxXKPnAmEqspZMhc4wo2EI/eh2WGic=; b=H7d9WAfdmwyj+r+BLQdHfZUKoNuvjNty85G1lBaw3rtZZR926TU2c91XP/R2bDOPDg ZPvGdHaXnOm2+CVHTWqA1t2hNnHb4P3AymoOqzQ88mlxqGXNfqUg+1oCDXw9paInLpbP 6Zz/M2r9VYJ9VJNbR6ttaWSwY3QRzlEA3vpaUXYUBgn0PsPxqn0zxdgsV1ClcVTD6kJ2 beNV5tY0Zu70uKEpsZNV1R81lxoWflfmnC0s7aNOSHwi1x90zysi/ExUYHHyXyGf2y5y N5+FfB5cdvt8JlL0ApTWep30v8YTQnei6r7mTR0o38/KCrRw+FteqLOHjJiBRe3imTOl cnJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:user-agent:references:message-id:in-reply-to:subject :cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=cbrmFiFJ7inYSjxXKPnAmEqspZMhc4wo2EI/eh2WGic=; b=AktZ+yaqtx9/LMeyjl8EBu1IIfgJjl7VncDN3NAO7o3AioYUiv7i1HsMY6bdT+y7gD cQqxUhbflYhU9lJAaSBL606pMUei1JgCZaH20cUgfyWbpywZpCbQWT+MuTMiUyitLXI6 ioYrv/1mlSX0ZctXQM1e9lg+VSTs/Nw/RDW/aTCbka/GZrZCS7+w2GS8nfQtHKnXRuwN WeAqWjcsB+pm/LKN9v9M4Uic4wcwcukMHGXTszGyO4Kq9ac7vU1UcIN47Z+k1oIHstte OsHZu0wUVc8AhK4u0iSoy4hKCwMJVj/0EU+gyoiqbpoPsz5Ttmp46m8qmgu594ULQovT IpDw== X-Gm-Message-State: AFqh2kqt4zZNkmNiFjicmDZmfVWx1LEoI65zDRPEbbqkEoOJEZhC9dHv Oucowpwsoa/JNAm2WZzvkXFiTcrlssf4iols X-Google-Smtp-Source: AMrXdXth5hB36DlU5YqP8JiPfFRhDuNd6hZWY7rRo/d8oKuhj78MM4xgrKHu6gQtJTz2DZFuXosrXA== X-Received: by 2002:a19:6a14:0:b0:4bc:af5:b8d9 with SMTP id u20-20020a196a14000000b004bc0af5b8d9mr5801599lfu.6.1674163056088; Thu, 19 Jan 2023 13:17:36 -0800 (PST) Received: from [192.168.219.3] ([78.8.192.131]) by smtp.gmail.com with ESMTPSA id g12-20020a0565123b8c00b004aa543f3748sm5975076lfv.130.2023.01.19.13.17.30 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 Jan 2023 13:17:32 -0800 (PST) Date: Thu, 19 Jan 2023 21:17:26 +0000 (GMT) From: "Maciej W. Rozycki" To: Simon Marchi cc: gdb-patches@sourceware.org, Andrew Burgess , Tom Tromey , Simon Sobisch Subject: Re: [PATCH v8 4/6] GDB: Allow arbitrary keywords in integer set commands In-Reply-To: <368aabac-7418-3323-841a-45a16f82b796@polymtl.ca> Message-ID: References: <368aabac-7418-3323-841a-45a16f82b796@polymtl.ca> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham 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 Thu, 12 Jan 2023, Simon Marchi wrote: > 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. Given your rationale I chose to commit this change, despite a concern with not using `setting', as this code is correct regardless and as you say we can always update it further. See below for details. Thank you for your review. > > + 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. It's actually used with the next patch and even covered in the testsuite (with 6/6): gdb_test "p \$_gdb_setting(\"print characters\")" " = void" \ "_gdb_setting print characters default" I didn't think it would make sense to move the implementation of this code path to the next patch even though this one does not make use of it. Overall I think it will be best if we do not expose internals and add new magic numbers now that we have the infrastructure that makes it possible to avoid. I only made an exception following Andrew's request here: to make `set print characters' accept "0" for "unlimited" for consistency with `set print elements'. > > + 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. Fixed, thanks. > > 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. Good catch, thanks! Fixed. > > -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. It is used by `parse_option' however, where no setting is involved, so it's not clear to me how to do that. I could make an artificial setting I suppose, but I feel like it's kind of creating a problem to fit the solution. Let me know if I've missed something. > > + 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. Fixed, thanks. > > @@ -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. Done (`enum var_types' above seems inconsistent about it). > > + }; > > The body of the struct should have one less indentation level (curly > braces at column 0). OK (again, just copied `enum var_types' style here). > > 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? I guess it's been an artefact from the previous approach which used `erased_args'. Updated accordingly now. > > +/* 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. Fixed. > > +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. Ditto. > > + if (var.type () == var_uinteger) > > + return scm_from_uint (static_cast (value)); > > + else > > + return scm_from_int (static_cast (value)); > > Unnecessary consts in casts. Fixed. > > + if (var_type == var_uinteger) > > + var.set (static_cast (val)); > > + else > > + var.set (static_cast (val)); > > Unnecessary consts in casts. Likewise. > > +/* 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. Fixed. > 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. There's no violation as they are defined in separate translation units each and both have local scope. And so do the respective enumeration constants. > > +/* 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. Fixed. > > + if (strcmp (l->literal, "unlimited") == 0) > > + { > > + /* Compatibility hack for API brokennes. */ > > brokenness Good catch, thanks! > > + 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. Fixed too. I'm posting the final version of the patch committed, in a reply to this message. Maciej