From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 78A353858D28 for ; Tue, 14 Dec 2021 11:53:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 78A353858D28 Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-44-3b4TFSE3OnWK4i_75K29Fw-1; Tue, 14 Dec 2021 06:53:02 -0500 X-MC-Unique: 3b4TFSE3OnWK4i_75K29Fw-1 Received: by mail-wr1-f70.google.com with SMTP id o4-20020adfca04000000b0018f07ad171aso4603911wrh.20 for ; Tue, 14 Dec 2021 03:53:02 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=//cMUNAHM+4JJY2aV5XX+Gl6PPUP5jDo2Q2WQhIoZH8=; b=iKeGRV4gw8T7dxE4UKC9dnOwLnwZRN83z/ToDPoBtD6SMTd/36gMVEIU853IHX01fs r+5icMAZ60ba4D94zXazrvIw/b7wf1fxT3giTze6ySM48nM1h3pgR/JjyuQH1WAkO2Jp zqItG85Uim2WGD/UQjwRXPPabFH3vK5By3mxp45oZ21G49xI3D6fxpPYhahhpM0qf+wo RhDLD7hLt9l7qFhZgtHokdEKjHf2ENeeKaKbReaxJ6mvKCkrPy1PuF4mPnRh3rEGHIrT MlwFuTk6IkQWaNiLZrJzb/6HeG18byrQpylB+zo4rV9sFt5wlRwqX8NMI8uNXXKnxkoR mmAA== X-Gm-Message-State: AOAM532OO3xJV3/ZD0BxJ7nhMK5GqTKym+UJUnDvwZVhQrjOd3URXsF1 Ss1nxCh9Sln7Xamhim7cMc+iy8vAcBNNP9RXZnxyulmJJmTpKJ8PvJ9r1apIKWBvvfPLV32WLff IJIqxR6K5WC+MI47Xpe/Sbg== X-Received: by 2002:a5d:6a4d:: with SMTP id t13mr5086819wrw.104.1639482780683; Tue, 14 Dec 2021 03:53:00 -0800 (PST) X-Google-Smtp-Source: ABdhPJwxI+6yNhx4GnUYJbxhyLjL5ukAefvPTaea26AyghaVdF5cvpncBpjktZgf8AWAsVaPEHIqjA== X-Received: by 2002:a5d:6a4d:: with SMTP id t13mr5086792wrw.104.1639482780384; Tue, 14 Dec 2021 03:53:00 -0800 (PST) Received: from localhost (host86-134-238-138.range86-134.btcentralplus.com. [86.134.238.138]) by smtp.gmail.com with ESMTPSA id j40sm2029726wms.16.2021.12.14.03.52.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Dec 2021 03:53:00 -0800 (PST) Date: Tue, 14 Dec 2021 11:52:59 +0000 From: Andrew Burgess To: Simon Marchi Cc: gdb-patches@sourceware.org, Jan Vrany Subject: Re: [PATCHv2 2/5] gdb/mi: use std::map for MI commands in mi-cmds.c Message-ID: <20211214115259.GD1831945@redhat.com> References: MIME-Version: 1.0 In-Reply-To: X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 11:48:59 up 1 day, 22:06, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Dec 2021 11:53:05 -0000 * Simon Marchi [2021-12-13 10:51:02 -0500]: > On 2021-12-03 11:29 a.m., Andrew Burgess via Gdb-patches wrote: > > From: Jan Vrany > > > > This changes the hashmap used in mi-cmds.c from a custom structure to > > std::map. Not only is replacing a custom container with a standard > > one an improvement, but using std::map will make it easier to > > dynamically add commands; which is something that is planned for a > > later series, where we will allow MI commands to be implemented in > > Python. > > > > There should be no user visible changes after this commit. > > LGTM with some minor comments, no need to repost just for that. > > > --- > > gdb/mi/mi-cmds.c | 478 +++++++++++++++++++++++------------------------ > > 1 file changed, 230 insertions(+), 248 deletions(-) > > > > diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c > > index 8899fdd3a1e..4999c9f81b3 100644 > > --- a/gdb/mi/mi-cmds.c > > +++ b/gdb/mi/mi-cmds.c > > @@ -22,283 +22,265 @@ > > #include "top.h" > > #include "mi-cmds.h" > > #include "mi-main.h" > > +#include > > +#include > > > > -struct mi_cmd; > > -static struct mi_cmd **lookup_table (const char *command); > > -static void build_table (struct mi_cmd *commands); > > +/* A command held in the MI_CMD_TABLE. */ > > > > -static struct mi_cmd mi_cmds[] = > > -{ > > -/* Define a MI command of NAME, and its corresponding CLI command is > > - CLI_NAME. */ > > -#define DEF_MI_CMD_CLI_1(NAME, CLI_NAME, ARGS_P, CALLED) \ > > - { NAME, { CLI_NAME, ARGS_P}, NULL, CALLED } > > -#define DEF_MI_CMD_CLI(NAME, CLI_NAME, ARGS_P) \ > > - DEF_MI_CMD_CLI_1(NAME, CLI_NAME, ARGS_P, NULL) > > +typedef std::unique_ptr mi_cmd_up; > > Really a nit: since we're using C++11, I prefer the notation `using = > ..`, I find it easier to read (since it looks like a standard > assignment, with the name on the left). I've made this change. > > > +/* Create an mi_cmd structure with name NAME. */ > > + > > +static mi_cmd_up > > +create_mi_cmd (const char *name) > > { > > - return *lookup_table (command); > > + mi_cmd_up cmd (new mi_cmd ()); > > + cmd->name = name; > > + return cmd; > > } > > It looks like this could be a constructor of mi_cmd quite easily, > instead of being a free function. I'm leaving this comment just for the record, I'm sure you saw this when you reviewed these later patches... I've not made any of the changes you suggested for moving code into the constructor within this patch. This code is all refactored as part of a later patch in this series, with the result that all initialisation is moved into the constructor(s) then. > > > > > -/* Used for collecting hash hit/miss statistics. */ > > +/* Create and register a new MI command with an MI specific implementation. > > + NAME must name an MI command that does not already exist, otherwise an > > + assertion will trigger. */ > > > > -static struct > > +static void > > +add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function, > > + int *suppress_notification = nullptr) > > { > > - int hit; > > - int miss; > > - int rehash; > > -} stats; > > + mi_cmd_up cmd_up = create_mi_cmd (name); > > + > > + cmd_up->cli.cmd = nullptr; > > + cmd_up->cli.args_p = 0; > > + cmd_up->argv_func = function; > > + cmd_up->suppress_notification = suppress_notification; > > ... and these could also be initialized in the mi_cmd constructor. Up > to you to see if you want to make that change, it can also be changed > later. As above. > > > > > -/* Look up a command. */ > > + bool success = insert_mi_cmd_entry (std::move (cmd_up)); > > + gdb_assert (success); > > +} > > + > > +/* Create and register a new MI command implemented on top of a CLI > > + command. NAME must name an MI command that does not already exist, > > + otherwise an assertion will trigger. */ > > > > -static struct mi_cmd ** > > -lookup_table (const char *command) > > +static void > > +add_mi_cmd_cli (const char *name, const char *cli_name, int args_p, > > + int *suppress_notification = nullptr) > > { > > - const char *chp; > > - unsigned int index = 0; > > + mi_cmd_up cmd_up = create_mi_cmd (name); > > > > - /* Compute our hash. */ > > - for (chp = command; *chp; chp++) > > - { > > - /* We use a somewhat arbitrary formula. */ > > - index = ((index << 6) + (unsigned int) *chp) % MI_TABLE_SIZE; > > - } > > + cmd_up->cli.cmd = cli_name; > > + cmd_up->cli.args_p = args_p; > > + cmd_up->argv_func = nullptr; > > + cmd_up->suppress_notification = suppress_notification; > > > > - while (1) > > - { > > - struct mi_cmd **entry = &mi_table[index]; > > - if ((*entry) == 0) > > - { > > - /* not found, return pointer to next free. */ > > - stats.miss++; > > - return entry; > > - } > > - if (strcmp (command, (*entry)->name) == 0) > > - { > > - stats.hit++; > > - return entry; /* found */ > > - } > > - index = (index + 1) % MI_TABLE_SIZE; > > - stats.rehash++; > > - } > > + bool success = insert_mi_cmd_entry (std::move (cmd_up)); > > + gdb_assert (success); > > } > > > > +/* Initialize MI_CMD_TABLE, the global map of MI commands. */ > > + > > static void > > -build_table (struct mi_cmd *commands) > > +build_table () > > Suggest renaming this to something like "add_builtin_mi_cmds", it would > be more descriptive. I've done this as an extra patch, which applies at the end of this series, I've gone ahead and pushed this new patch. The patch is included below, for the record. Thanks, Andrew --- commit 78d4da9ae0d3447f28274a00b278f58ca7d8d1b2 Author: Andrew Burgess Date: Tue Dec 14 11:43:34 2021 +0000 gdb/mi: rename build_table to add_builtin_mi_commands Just give the function build_table a more descriptive name. There should be no user visible changes after this commit. diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c index 9c11db00b3a..d02e4b20562 100644 --- a/gdb/mi/mi-cmds.c +++ b/gdb/mi/mi-cmds.c @@ -190,7 +190,7 @@ mi_command::do_suppress_notification () const /* Initialize the available MI commands. */ static void -build_table () +add_builtin_mi_commands () { add_mi_cmd_mi ("ada-task-info", mi_cmd_ada_task_info); add_mi_cmd_mi ("add-inferior", mi_cmd_add_inferior); @@ -368,5 +368,5 @@ void _initialize_mi_cmds (); void _initialize_mi_cmds () { - build_table (); + add_builtin_mi_commands (); }