From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gateway34.websitewelcome.com (gateway34.websitewelcome.com [192.185.148.140]) by sourceware.org (Postfix) with ESMTPS id 28B86398B862 for ; Wed, 9 Jun 2021 15:02:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 28B86398B862 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tromey.com Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=tromey.com Received: from cm13.websitewelcome.com (cm13.websitewelcome.com [100.42.49.6]) by gateway34.websitewelcome.com (Postfix) with ESMTP id 40DE4147A87 for ; Wed, 9 Jun 2021 10:02:10 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id qzicl0p4SVBxyqziclQ3Bk; Wed, 09 Jun 2021 10:02:10 -0500 X-Authority-Reason: nr=8 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date:References :Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=oz6mQ+HTEiLVeKZwofPhrfpmjTZe8KKuyZPe8iZCgYk=; b=rbkUV1rUAxpk79Llu/ajQFIwrL 8xp5PhkgZqW+8daaMlJwMZ0Ftk3n6+Fm6ye715nU8VhhkOG94S3/yuu3U5ThGWYpPFs0ebOdPVxRR 7+90bl/+/L8ZNwAEtYfBCkr6k; Received: from 97-122-81-113.hlrn.qwest.net ([97.122.81.113]:58596 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1lqzib-003Lkj-KH; Wed, 09 Jun 2021 09:02:09 -0600 From: Tom Tromey To: Vasili Burdo via Gdb-patches Subject: Re: [PATCH] TUI disassembly window improvement - patch inline References: X-Attribution: Tom Date: Wed, 09 Jun 2021 09:02:08 -0600 In-Reply-To: (Vasili Burdo via Gdb-patches's message of "Tue, 8 Jun 2021 19:21:51 +0300") Message-ID: <878s3jjd3j.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - box5379.bluehost.com X-AntiAbuse: Original Domain - sourceware.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - tromey.com X-BWhitelist: no X-Source-IP: 97.122.81.113 X-Source-L: No X-Exim-ID: 1lqzib-003Lkj-KH X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 97-122-81-113.hlrn.qwest.net (murgatroyd) [97.122.81.113]:58596 X-Source-Auth: tom+tromey.com X-Email-Count: 1 X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-Spam-Status: No, score=-3026.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, JMQ_SPF_NEUTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_NEUTRAL, TXREP autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 09 Jun 2021 15:02:16 -0000 >>>>> "Vasili" == Vasili Burdo via Gdb-patches writes: Vasili> I implemented a small update to TUI disassembly window - see attachment. Vasili> This patch removes function name from disassembly listing leaving only Vasili> offset from function start. Vasili> The reason for it - disassembly TUI is almost unusable in case of C++ Vasili> functions - their names (especially templated ones) are very long and Vasili> thus litter disassembly view. Hi. Thank you for your patch. I like the idea, I think it's a nice improvement. Currently, patches require a ChangeLog entry. See https://sourceware.org/gdb/wiki/ContributionChecklist Also, if you do not have a copyright assignment, then you will probably need one. You can email me off-list to get started. We can't check in a non-trivial patch without one. Vasili> Please, look at sample screenshots here: https://imgur.com/a/GlkVXGi I appreciate this, but I think it would be better if the commit message didn't reference a URL like this. Perhaps a description would be better. This is "PR tui/25347" (see https://sourceware.org/bugzilla/show_bug.cgi?id=25347) so the commit message ought to include that text. This will ensure the commit is mentioned in the bug. Vasili> @@ -110,7 +113,7 @@ tui_disassemble (struct gdbarch *gdbarch, Vasili> asm_lines.clear (); Vasili> /* Now construct each line. */ Vasili> - for (int i = 0; i < count; ++i) Vasili> + while (asm_lines.size() < count) gdb style is a space before an open paren. Vasili> + if(for_ui) I didn't really understand why this function needed two modes. I think it would be good to update the introductory comment before tui_disassemble to explain the meaning of the new parameter. Vasili> + if (0 == build_address_symbolic (gdbarch, orig_pc, Vasili> asm_demangle, false, Your mailer seems to have mangled the patch. This will make it harder to eventually apply it. Vasili> + /* Set title to current function name */ Vasili> + title.clear(); Vasili> + if (asm_lines.size()) Vasili> + title = asm_lines.front().func_string; Probably you can check '!asm_lines.empty ()' instead. I wonder if the title setting is correct here. What if the window contents end up like: fn1: asm asm asm fn2: asm PC => asm asm In this scenario, would the title be "fn1" or "fn2"? Also, which is better? On the one hand, if the title just shows the function at the top of the window, then scrolling down will work nicely in a way -- when a function name scrolls off the top, it will stick in the title. On the other hand, I might expect the title to display the function holding the PC. I'm really not sure, your input is appreciated. I didn't try the patch, but it would be good to report how you tested it. I don't recall offhand if there are TUI tests that would need to be updated. When modifying the TUI like this, it's usually fine to just re-run the gdb.tui tests. If you're unfamiliar with dejagnu, just ask and I can give you a recipe. thanks again, Tom