From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id m2BNAzp0zF/pawAAWB0awg (envelope-from ) for ; Sun, 06 Dec 2020 01:03:38 -0500 Received: by simark.ca (Postfix, from userid 112) id 00D3C1F0B8; Sun, 6 Dec 2020 01:03:37 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 16FCA1E99A for ; Sun, 6 Dec 2020 01:03:37 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id BEB573861810; Sun, 6 Dec 2020 06:03:36 +0000 (GMT) Received: from rock.gnat.com (rock.gnat.com [IPv6:2620:20:4000:0:a9e:1ff:fe9b:1d1]) by sourceware.org (Postfix) with ESMTP id 064DC3861810 for ; Sun, 6 Dec 2020 06:03:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 064DC3861810 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=brobecker@adacore.com Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id AF555116B9E; Sun, 6 Dec 2020 01:03:33 -0500 (EST) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id VfrxNXB1JkoL; Sun, 6 Dec 2020 01:03:33 -0500 (EST) Received: from float.home (localhost.localdomain [127.0.0.1]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id 51301116B84; Sun, 6 Dec 2020 01:03:33 -0500 (EST) Received: by float.home (Postfix, from userid 1000) id 0609FA1880; Sun, 6 Dec 2020 10:03:28 +0400 (+04) Date: Sun, 6 Dec 2020 10:03:27 +0400 From: Joel Brobecker To: Simon Marchi via Gdb-patches Subject: Re: [PATCH 2/4] gdb: make get_discrete_bounds return bool Message-ID: <20201206060327.GA327270@adacore.com> References: <20201123162120.3778679-1-simon.marchi@efficios.com> <20201123162120.3778679-3-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201123162120.3778679-3-simon.marchi@efficios.com> 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: , Cc: Simon Marchi Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" Hi Simon, On Mon, Nov 23, 2020 at 11:21:18AM -0500, Simon Marchi via Gdb-patches wrote: > get_discrete_bounds currently has three possible return values (see its > current doc for details). It appears that for all callers, it would be > sufficient to have a boolean "worked" / "didn't work" return value. > > Change the return type of get_discrete_bounds to bool and adjust all > callers. Doing so simplifies the following patch. > > gdb/ChangeLog: > > * gdbtypes.h (get_discrete_bounds): Return bool, adjust all > callers. > * gdbtypes.c (get_discrete_bounds): Return bool. > > Change-Id: Ie51feee23c75f0cd7939742604282d745db59172 A small reminder to remember to remove the Change-Id... Other than that, the change looks good to me. This is a very nice simplification of the interface, IMO, so thank you for doing that. In reading the documentation prior to this change, it was hard for me to wrap my head around what it the function was really doing. In trying to understand the initial motivation, I did a bit of archeology, and it goes all the way back to the initial creation of the sourceware repository (20 years ago already!), so no obvious explanation from there. > @@ -399,7 +399,7 @@ m2_language::value_print_inner (struct value *val, struct ui_file *stream, > > fputs_filtered ("{", stream); > > - i = get_discrete_bounds (range, &low_bound, &high_bound); > + i = get_discrete_bounds (range, &low_bound, &high_bound) ? 0 : -1; > maybe_bad_bstring: > if (i < 0) > { FTR, this hunk required a bit more context to investigate. We can see that the change looks correct when looking at how variable "i" is (mis)used: i = get_discrete_bounds (range, &low_bound, &high_bound) ? 0 : -1; maybe_bad_bstring: if (i < 0) { fputs_styled (_(""), metadata_style.style (), stream); goto done; } for (i = low_bound; i <= high_bound; i++) Because of the use of labels, it's hard to propose a simpler rewriting which one would feel confident about without testing... > diff --git a/gdb/p-valprint.c b/gdb/p-valprint.c > index 428b2efc656..8f785b71ea4 100644 > --- a/gdb/p-valprint.c > +++ b/gdb/p-valprint.c > @@ -343,7 +343,8 @@ pascal_value_print_inner (struct value *val, struct ui_file *stream, > > fputs_filtered ("[", stream); > > - int bound_info = get_discrete_bounds (range, &low_bound, &high_bound); > + int bound_info = (get_discrete_bounds (range, &low_bound, &high_bound) > + ? 0 : -1); > if (low_bound == 0 && high_bound == -1 && TYPE_LENGTH (type) > 0) > { > /* If we know the size of the set type, we can figure out the Similar story here. -- Joel