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 1EE4F385701C for ; Wed, 21 Jun 2023 16:08:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1EE4F385701C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1687363687; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YNTzweG6sDkvcJqK3YljnzWKi79HMuQ/0VJEuvXJLjI=; b=EcQuS1XJxg31/KUAZrikm14+9fWV2Ef0E/C2nsUu4ULIbtHh4kSEQJjBCZPBsC/ibe3nQA hcnVVwK4WBju8Yrx0y4K9QTAhZC3b6Xm+c9OGMhRF/YojgL2zhP5CaHHjx0OPjQ3dUcfs0 YYArF/rmxFbdSsR1M/QTjupZGbaJhtw= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-674-NwhaSMz3OKSfRTEfXfxI8A-1; Wed, 21 Jun 2023 12:07:31 -0400 X-MC-Unique: NwhaSMz3OKSfRTEfXfxI8A-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id BEA372808E73 for ; Wed, 21 Jun 2023 16:07:21 +0000 (UTC) Received: from [10.22.18.131] (unknown [10.22.18.131]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E23F2492C13; Wed, 21 Jun 2023 16:07:20 +0000 (UTC) Message-ID: <2c50787b-6aa3-382e-37c4-7494da0a26c5@redhat.com> Date: Wed, 21 Jun 2023 09:07:19 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [PATCH v3 2/4] gdb/cli: Improve UX when using list with no args To: Bruno Larsen , gdb-patches@sourceware.org References: <20230621104545.2530552-1-blarsen@redhat.com> <20230621104545.2530552-3-blarsen@redhat.com> From: Keith Seitz In-Reply-To: <20230621104545.2530552-3-blarsen@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 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=-11.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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: Hi, Apologies, I'm late to the review here, so if I'm stepping on other's toes, please ignore me! On 6/21/23 03:45, Bruno Larsen via Gdb-patches wrote: > When using "list" with no arguments, GDB will first print the lines > around where the inferior is stopped, then print the next N lines until > reaching the end of file, at which point it wanrs the user "Line X out > of range, file Y only has X-1 lines.". This is usually desireable, but > if the user can no longer see the original line, they may have forgotten > the current line or that a list command was used at all, making GDB's > error message look cryptic. It was reported in bugzilla as PR cli/30497. I've run into this myself over the years. While I've adapted to it, you (and the original bz reporter) have a very valid point. This is simply not very user-friendly. So thank you for posting something to address this. > This commit improves the user experince by changing the behavior of > "list" slightly when a user passes no arguments. If the line that would > be printed is past the end of the file, GDB will now warn that the > previous list command has reached the end of file, and the current line > wil be listed again. [If this is to be your commit message, please note there are several typos in these two paragraphs ("desireable", "experince").] An example before(?) / after would help frame the discussion (see below). > diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c > index b0b9c08c2ec..5973aebfad3 100644 > --- a/gdb/cli/cli-cmds.c > +++ b/gdb/cli/cli-cmds.c > @@ -1244,8 +1244,40 @@ list_command (const char *arg, int from_tty) > list_around_line (arg, cursal); > } > > - /* "l" or "l +" lists next ten lines. */ > - else if (arg == NULL || arg[0] == '+') > + /* "l" lists the next few lines, unless we're listing past the end of > + the file. If it would be past the end, re-print the current line. */ > + else if (arg == nullptr) > + { > + if (can_print_line (cursal.symtab, cursal.line)) > + print_source_lines (cursal.symtab, > + source_lines_range (cursal.line), 0); > + else > + { > + warning (_("previous list command has already reached the end " > + "of the file. Listing default location again")); [Warning: You may want to skip this nit-picking and jump to "HOWEVER" below!] Nit: Grep'ping through the code, I see that some warnings are implemented as full sentences and some as incomplete sentences/phrases. This is both. I'm not a fan. I would prefer full sentences or "previous list command has already reached the end of the file -- listing default location again". On that front, I'm not sure I like the whole "Listing default location again" phrase. Do users typically know what a "default" location is? I don't see this term specifically defined in either the manual or the proposed patches? [I see there is a definition in NEWS which seems insufficient.] HOWEVER, if I may offer up an example around which to discuss things... Current behavior, using "list main.c:0" on gdb's sources, and simply hitting [Enter] until the behavior of interest appears: (top-gdb) 1481 gdb_printf (stream, _("\n\ 1482 Report bugs to %ps.\n\ 1483 "), styled_string (file_name_style.style (), REPORT_BUGS_TO)); 1484 if (stream == gdb_stdout) 1485 gdb_printf (stream, _("\n\ 1486 You can ask GDB-related questions on the GDB users mailing list\n\ 1487 (gdb@sourceware.org) or on GDB's IRC channel (#gdb on Libera.Chat).\n")); 1488 } (top-gdb) Line number 1489 out of range; ../../src/gdb/main.c has 1488 lines. (top-gdb) The proposal here changes this to: (top-gdb) 1481 gdb_printf (stream, _("\n\ 1482 Report bugs to %ps.\n\ 1483 "), styled_string (file_name_style.style (), REPORT_BUGS_TO)); 1484 if (stream == gdb_stdout) 1485 gdb_printf (stream, _("\n\ 1486 You can ask GDB-related questions on the GDB users mailing list\n\ 1487 (gdb@sourceware.org) or on GDB's IRC channel (#gdb on Libera.Chat).\n")); 1488 } (top-gdb) warning: previous list command has already reached the end of the file. Listing default location again 14 GNU General Public License for more details. 15 16 You should have received a copy of the GNU General Public License 17 along with this program. If not, see . */ 18 19 #include "defs.h" 20 #include "main.h" 21 #include "interps.h" 22 23 int (top-gdb) TBH, I was surprised by this. If a user was looking at the end of a source file, would they really want to suddenly jump to the top? [They could always "list 0" to easily accomplish that.] Also note that as implemented in these patches, hitting [Enter] AGAIN gets "stuck" in a weird way [this scenario needs tests]: (top-gdb) 1481 gdb_printf (stream, _("\n\ 1482 Report bugs to %ps.\n\ 1483 "), styled_string (file_name_style.style (), REPORT_BUGS_TO)); 1484 if (stream == gdb_stdout) 1485 gdb_printf (stream, _("\n\ 1486 You can ask GDB-related questions on the GDB users mailing list\n\ 1487 (gdb@sourceware.org) or on GDB's IRC channel (#gdb on Libera.Chat).\n")); 1488 } (top-gdb) warning: previous list command has already reached the end of the file. Listing default location again 14 GNU General Public License for more details. 15 16 You should have received a copy of the GNU General Public License 17 along with this program. If not, see . */ 18 19 #include "defs.h" 20 #include "main.h" 21 #include "interps.h" 22 23 int (top-gdb) 24 main (int argc, char **argv) 25 { 26 struct captured_main_args args; 27 28 memset (&args, 0, sizeof args); 29 args.argc = argc; 30 args.argv = argv; 31 args.interpreter_p = INTERP_CONSOLE; 32 return gdb_main (&args); 33 } (top-gdb) warning: previous list command has already reached the end of the file. Listing default location again 14 GNU General Public License for more details. 15 16 You should have received a copy of the GNU General Public License 17 along with this program. If not, see . */ 18 19 #include "defs.h" 20 #include "main.h" 21 #include "interps.h" 22 23 int (top-gdb) The "stuck" behavior/bug here aside, I would like to suggest a simpler solution: list the last N lines of the file when attempting to list past the last line. Something like (completely uncoded/artificially created): (top-gdb) 1481 gdb_printf (stream, _("\n\ 1482 Report bugs to %ps.\n\ 1483 "), styled_string (file_name_style.style (), REPORT_BUGS_TO)); 1484 if (stream == gdb_stdout) 1485 gdb_printf (stream, _("\n\ 1486 You can ask GDB-related questions on the GDB users mailing list\n\ 1487 (gdb@sourceware.org) or on GDB's IRC channel (#gdb on Libera.Chat).\n")); 1488 } (top-gdb) Line number 1489 out of range; ../../src/gdb/main.c has 1488 lines. 1481 gdb_printf (stream, _("\n\ 1482 Report bugs to %ps.\n\ 1483 "), styled_string (file_name_style.style (), REPORT_BUGS_TO)); 1484 if (stream == gdb_stdout) 1485 gdb_printf (stream, _("\n\ 1486 You can ask GDB-related questions on the GDB users mailing list\n\ 1487 (gdb@sourceware.org) or on GDB's IRC channel (#gdb on Libera.Chat).\n")); 1488 } [and keep repeating this behavior until a location is explicitly given by the user] That keeps the user focused at the end of the file. [AFAIK, we have no convenient/easy way to jump to the end of a file.] WDYT? Keith PS. OOC do you know if changing this list-past-end-of-file behavior alters the way MI currently behaves?