From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f42.google.com (mail-wr1-f42.google.com [209.85.221.42]) by sourceware.org (Postfix) with ESMTPS id 06AB738493D8 for ; Fri, 13 Jan 2023 14:45:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 06AB738493D8 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f42.google.com with SMTP id q10so1962325wrs.2 for ; Fri, 13 Jan 2023 06:45:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=5cPo3q1TsphVu15pOKG4qtOwWth2sxa4G/vxAlPhUFE=; b=j80jqwIuhzJXd3tL/2BrIVKcbEqrNxh7SqxNQSg8nY1MMtjuQf7+ADHoYv5HXgol0f enBv/vPdKY7uI0pcwL8QY5a+7wHY8WlRHYCOA39H2CSnQgfqP+kolWnZ95Ka9dO4Xl6s uTYMno6do2t9GStsDPZL+vtJLq+rXKGofHsuWqHJB7EgNQwK1BD5w24iOGV5g88de0vb KEOXOlfG/xEOMslrqVR/75oLCJ+jfp5/jp8rLpzYyY3osjd3nZ8hJs/JG2xlr+G3yOs6 dQjBagivpnZ37yClDp/wkqW9ESnCB+moIYAorcKWeRKwfpY6VJYbEDBYfrDsk2aCuyQS 3Q4Q== X-Gm-Message-State: AFqh2krqMYaatoDeGSaPBl2uN0le5k1DtYKqUhKwBvMW4svQFjOt/RqO Aj035xLrbByzR5aTgsDxUKLNSw4qA8mb1A== X-Google-Smtp-Source: AMrXdXvqA7fxXGPfUtKwBmLnJNWQStyjQKf5NUetQ0vSXcO1gZ6WNs2y17Hn+OHr9WY5ACDS2lqpyg== X-Received: by 2002:a5d:5548:0:b0:2bc:6947:75d6 with SMTP id g8-20020a5d5548000000b002bc694775d6mr11587078wrw.27.1673621114417; Fri, 13 Jan 2023 06:45:14 -0800 (PST) Received: from ?IPv6:2001:8a0:f92b:9e00::1fe? ([2001:8a0:f92b:9e00::1fe]) by smtp.gmail.com with ESMTPSA id l13-20020adfe58d000000b00296730b5c3esm19279155wrm.102.2023.01.13.06.45.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 13 Jan 2023 06:45:14 -0800 (PST) Subject: Re: [PATCH v2 79/79] Consolidate calls to require To: Tom Tromey Cc: gdb-patches@sourceware.org References: <20230112030052.3306113-1-tom@tromey.com> <20230112030052.3306113-80-tom@tromey.com> <2e1baa96-5289-537f-5bb4-f128056f6583@palves.net> <87y1q64jf9.fsf@tromey.com> From: Pedro Alves Message-ID: <383f7993-e347-f6c1-ee83-0de1da10dbe2@palves.net> Date: Fri, 13 Jan 2023 14:45:13 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <87y1q64jf9.fsf@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2023-01-13 2:07 p.m., Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves writes: > >>> -require support_displaced_stepping >>> - >>> # The testfile uses "run". The real bug happened only for ![is_remote target]. >>> -require !use_gdb_stub >>> +require support_displaced_stepping !use_gdb_stub > > Pedro> I'm not really sure consolidation when we have a comment describing each "require" line > Pedro> before is an improvement. For example, above, it was clear before that the use_gdb_stub > Pedro> requirement was related to "run". Afterwards, it is not clear whether the > Pedro> support_displaced_stepping check is related to the comment. > > TBH I think the comments should generally just be removed, because the > "require" line seems like sufficient explanation to me. WDYT? I maybe agree in some cases, but not in general. The reason for using use_gdb_stub is not always obvious. For example, see the comments on gdb.base/solib-display.exp. Here's another example, this one from gdb.base/fork-print-inferior-events.exp. Before your patch, we have: # This test relies on "run", so it cannot run on target remote stubs. require !use_gdb_stub # Test relies on checking follow-fork output. Do not run if gdb debug is # enabled as it will be redirected to the log. require !gdb_debug_enabled which look better to me than this, after your patch: # This test relies on "run", so it cannot run on target remote stubs. # Test relies on checking follow-fork output. Do not run if gdb debug is # enabled as it will be redirected to the log. require !use_gdb_stub !gdb_debug_enabled as with the latter you have to think about which expression matches each part of the comment, and vice versa. Is the sentence in the middle connected to use_gdb_stub, or to gdb_debug_enabled? We lose the comment-expression locality. (Also, the comment about gdb_debug_enabled is far from obvious, IMO.) I just would not like to enforce an unwritten rule that we should always consolidate require calls. Is there an advantage of consolidation in these cases?