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.129.124]) by sourceware.org (Postfix) with ESMTPS id 4B7213858D39 for ; Wed, 19 Oct 2022 08:31:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4B7213858D39 Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-93-MAJzpVjhPaqlP9IcN0VGYA-1; Wed, 19 Oct 2022 04:31:01 -0400 X-MC-Unique: MAJzpVjhPaqlP9IcN0VGYA-1 Received: by mail-qv1-f72.google.com with SMTP id q17-20020a056214019100b004b1d3c9f3acso10144476qvr.0 for ; Wed, 19 Oct 2022 01:31:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=KhMrQDyK8iQAlpMH8bDWFrKmR7GNQeelXM0ZNPqB/3s=; b=c6Xtq/bdVZ9Ywk8edbjLqm/z23hM5ssCrVE06AaFnRIcWs9BBbRx1/XydEA4mQSk38 nxPPrcRurzh76MTugyVYtI/J4aQhpTCgYVx50J/Y6FMl+eqZsjeuz2lKko7iwUsxFp+r daytmuSeC0I0Y6yvcxUEY1DgtFU7ijZIZUUw3FVBrbv1Pu1P+NPOaB0bpzDR90ick8Sy DHYl5bp1RMT1ycCRGcgTB1m6hW6NkbgM+AxxDaeXQCQNGQRASV7o3MZQtiWeRSeaWAV2 BMRgB4e4D3zZ9A3WozgC7eju0NJTEiIxMfGbmfdMhWMkt5e/OITzCt6M6suz3/QeEGPe CxSg== X-Gm-Message-State: ACrzQf15P1ZyvkjUGKhKfeBpEn2CqObRfbrQ7fjbsVwPLZiDAbr50a7e XtnHUIiprO5NyGtEBLLwr/h/v0z9P9vNHGpJHjzg6k/cBj7i/6CiE3n26rj4AAmDDSQvu90PUoh S7hL1tocnvAQwmZuSJ1gTdA== X-Received: by 2002:a05:6214:5cc5:b0:4b3:ec9e:79d8 with SMTP id lk5-20020a0562145cc500b004b3ec9e79d8mr5559915qvb.61.1666168260448; Wed, 19 Oct 2022 01:31:00 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4Tm0uUJh/Mz+fBfWZMx9ofP1Ct6eygyCYPtEPdAphP35iAov392h8qz6a3cPh6fEpTjS0PHA== X-Received: by 2002:a05:6214:5cc5:b0:4b3:ec9e:79d8 with SMTP id lk5-20020a0562145cc500b004b3ec9e79d8mr5559904qvb.61.1666168260161; Wed, 19 Oct 2022 01:31:00 -0700 (PDT) Received: from [10.43.2.105] (nat-pool-brq-t.redhat.com. [213.175.37.10]) by smtp.gmail.com with ESMTPSA id cj24-20020a05622a259800b00399fe4aac3esm3638287qtb.50.2022.10.19.01.30.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 19 Oct 2022 01:30:59 -0700 (PDT) Message-ID: <6e891af5-eb49-363f-a765-a22d7a048e12@redhat.com> Date: Wed, 19 Oct 2022 10:30:57 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 Subject: Re: [RFC PATCH] gdb: add command to clear value history To: Rustam Kovhaev , gdb-patches@sourceware.org References: <20221019033925.86910-1-rkovhaev@gmail.com> From: Bruno Larsen In-Reply-To: <20221019033925.86910-1-rkovhaev@gmail.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, 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 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: Wed, 19 Oct 2022 08:31:04 -0000 On 19/10/2022 05:39, Rustam Kovhaev via Gdb-patches wrote: > Hello, Hi Rustam! Thanks for this patch! I am indifferent to the new command, as I never needed it but I can't see it impacting anything else, so I'm fine with it being added. However, the command "clear" already exists. It clears all breakpoints. You could either add "values" as a subcommand to the existing command, by having it added around gdb/breakpoint.c:14434, but doing so might change how the bare "clear" command works (I'm not sure here), or add something like "values clear" instead, which I think it's a bit too big. This specific point I'll defer to other maintainers on helping you. I also noticed a few specific things that I'll explain inline. > While debugging an application I wanted to clear the value history and I > could not find an option to do it. > I might be the only one who gets lost in the value history and I am not > sure if anyone else needs it, but I thought it would be nice to have it. > And I hope this does not break anything. > Please let me know what you think. Thanks! > > Let's have the functionality to clear the value history. > Introduce 'set values clear' command to clear the value history. GDB tends to prefer a more objective commit message style, even if it comes from a personal need. Your message is fine for the RFC but it needs to be iterated for it to go in. See commits e4014689b9a1b9aa0dde8f8a358401774566fe8b and 70175292616118bad315296ba6180f375326bb6c for some exaples. > > Signed-off-by: Rustam Kovhaev We don't really use Signed-off-by tags in GDB. > --- > gdb/value.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/gdb/value.c b/gdb/value.c > index 605e52dee82..726b5ef9084 100644 > --- a/gdb/value.c > +++ b/gdb/value.c > @@ -1971,6 +1971,15 @@ show_values (const char *num_exp, int from_tty) > if (from_tty && num_exp) > set_repeat_arguments ("+"); > } > + > +static struct cmd_list_element *setvallist; > + > +static void > +clear_values (const char *ignore, int from_tty) > +{ > + value_history.clear (); > +} > + > > enum internalvar_kind > { > @@ -4343,6 +4352,13 @@ Convenience functions are defined via the Python API." > Elements of value history around item number IDX (or last ten)."), > &showlist); > > + add_cmd("clear", no_class, clear_values, _("\ > +Clear value history."), &setvallist); You forgot to add a space before ( here. > + > + add_basic_prefix_cmd ("values", class_support, _("\ > +Generic command for setting value history parameters"), > + &setvallist, 0, &setlist); > + Since you are using &setvallist and &setlist here, your command ends up being "set values clear" instead of "clear values". You probably want to use &cmdlist instead. > add_com ("init-if-undefined", class_vars, init_if_undefined_command, _("\ > Initialize a convenience variable if necessary.\n\ > init-if-undefined VARIABLE = EXPRESSION\n\ It would be good if you added a test for this command. I think it would go well in gdb.base/printcmd.exp, adding some extra lines like gdb_test_no_output "clear values" "test clearing values" gdb_test "print 1" ".1 = 1" "test that values were cleared" With all this being said, I like the direction of your patch, I hope to see it going upstream soon :-) Cheers, Bruno