From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id fz6QJ+X5kl/qZAAAWB0awg (envelope-from ) for ; Fri, 23 Oct 2020 11:42:29 -0400 Received: by simark.ca (Postfix, from userid 112) id 9501A1EFC7; Fri, 23 Oct 2020 11:42:29 -0400 (EDT) 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 E22131E98E for ; Fri, 23 Oct 2020 11:42:28 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 61503398B40B; Fri, 23 Oct 2020 15:42:28 +0000 (GMT) Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id B2CBD385801D for ; Fri, 23 Oct 2020 15:42:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B2CBD385801D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 519341E552; Fri, 23 Oct 2020 11:42:26 -0400 (EDT) Subject: Re: [committed][gdb/symtab] Make find_block_in_blockvector more robust To: Tom de Vries , Tom Tromey References: <20201022152431.GA13910@delia> <87ft66144u.fsf@tromey.com> <8d84b50e-530a-a33c-b523-c72305438c19@suse.de> <4052e022-30fd-b24e-2ad6-53eb4d790d85@simark.ca> <8204bf8a-40f7-de0f-96a4-714f4db06d64@suse.de> From: Simon Marchi Message-ID: <59e4b623-6ef4-d7eb-8f9a-ecef76d066f8@simark.ca> Date: Fri, 23 Oct 2020 11:42:25 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <8204bf8a-40f7-de0f-96a4-714f4db06d64@suse.de> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit 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: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2020-10-23 10:08 a.m., Tom de Vries wrote: > Thanks for the feedback. > > I agree it would make it easier to read, but I'm afraid it would make it > harder to understand. > > In general I prefer range tests to conform to this layout: > ... > min cmp-op-1 val && val cmp-op-2 max > ... > which IMO is the best approximation of the non-C: > ... > min cmp-op-1 val cmp-op-2 max > ... > and then use the range expression as a whole, either negated or not. I think we all picture things a bit differently in our minds, which means we prefer some form over the other, and that's fine. Not trying to convince you to change anything, but just for the sake of discussion, this is how I picture it. When reading conditions, I translate to natural language in my head. Between: if the pc is smaller than the start of the block, then abort and if the start of the block is not smaller or equal than the pc In the first case, I understand in an instant "ok, it means if the pc is before the range". For the second, it takes me a little pause: "ok, so if the start of the block is smaller or equal to the pc, it means we're in range, so if _not_ that, it means we're out of range.". > > But indeed, the negated case is a bit harder to read, something I > sometimes fix by using a variable with a somewhat helpful name. > > This follows my preferred layout for the range expression (albeit split > up into two variables), while not negating non-trivial expressions: > ... > while (bot >= STATIC_BLOCK) > { > b = BLOCKVECTOR_BLOCK (bl, bot); > bool start_ok = BLOCK_START (b) <= pc; > bool end_ok = pc < BLOCK_END (b); > if (!start_ok) > return NULL; > if (end_ok) > return b; > bot--; > } > ... I like this version with variable names. The variable names convey some information about the intention of the code, and act as some kind of "checkpoints". If you understand what these boolean variables are meant to represent (hence the name must be clear), you don't need to parse the condition themselves to understand the algorithm. If you have some reason to believe the conditions are erroneous, then you can dig into them, but otherwise you can skip them and save some mental cycles. Of course, it gets more valuable as the size/complexity of the conditions increase. Simon