From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f54.google.com (mail-wr1-f54.google.com [209.85.221.54]) by sourceware.org (Postfix) with ESMTPS id EC3A33858D32 for ; Fri, 13 Jan 2023 12:48:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EC3A33858D32 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-f54.google.com with SMTP id z5so19907241wrt.6 for ; Fri, 13 Jan 2023 04:48:51 -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:to:subject :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=u9Na1P+MOpj8NpWJxe+lzqCIUjqR9sZ0VcToIkOuLLU=; b=svHNJyqSHUQn4TDbJt9ZltGtSQgzq8dWFliiHLIjafVBrA2sk2MmeeA1L/Iza+MlO/ jInsOGGqRtnlYCfeLGd+7DX/Fx52ZjiC41DScfhNsLgfsGo+e0XH6nhiNBI+UmBLiz8g wM9bI1DF3mFCXJ0tRNlbYps8I1d7dS3hjkeSs9+bvGtr9ZpApDfGlikieU6PqRKnBft2 FlE0nkvNpmywx5owAcFUqdCcGt3R3L2sXLAmFn68l3jZDKYdtZtkTvUh1bwYzMFV+cH1 e1BXDs85iND+gSSmvcYrOmeFsz1uT6j1XLVCiVwvmcGv2yVC0VJ3T07+VDsotDTqFUY/ 8NSw== X-Gm-Message-State: AFqh2krAwQFP8zUPQrE3MT7yUvpSFfjceQXJCSUjqZNxred4wKRFYDf9 QudB3QG7hphQGGgSRQAdF/wFsqN+XXiE5A== X-Google-Smtp-Source: AMrXdXtpTTQ0i0U0Qzx0EglXyIJ7M+QJe/KeKH2K0sWeItjQHDlVanJmcmp5RrfHSHANHh3B+v41Jw== X-Received: by 2002:a05:6000:810:b0:29f:9832:ff96 with SMTP id bt16-20020a056000081000b0029f9832ff96mr27158761wrb.2.1673614130408; Fri, 13 Jan 2023 04:48:50 -0800 (PST) Received: from ?IPv6:2001:8a0:f92b:9e00::1fe? ([2001:8a0:f92b:9e00::1fe]) by smtp.gmail.com with ESMTPSA id m13-20020adfe94d000000b002714b3d2348sm18950210wrn.25.2023.01.13.04.48.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 13 Jan 2023 04:48:50 -0800 (PST) Subject: Re: [PATCH v2 79/79] Consolidate calls to require To: Tom Tromey , gdb-patches@sourceware.org References: <20230112030052.3306113-1-tom@tromey.com> <20230112030052.3306113-80-tom@tromey.com> From: Pedro Alves Message-ID: <2e1baa96-5289-537f-5bb4-f128056f6583@palves.net> Date: Fri, 13 Jan 2023 12:48:49 +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: <20230112030052.3306113-80-tom@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP 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 Tom, Thanks much for this series. It's a nice improvement. I have a comment below. On 2023-01-12 3:00 a.m., Tom Tromey wrote: > The previous transforms left some files with multiple calls to > "require" near the top. This patch consolidates these to the extent > possible. (There are a couple of files that call "require" later, > after running some tests. These have been left alone.) > > diff --git a/gdb/testsuite/gdb.ada/catch_ex_std.exp b/gdb/testsuite/gdb.ada/catch_ex_std.exp > index 73cbdaf90ca..c7b65098208 100644 > --- a/gdb/testsuite/gdb.ada/catch_ex_std.exp > +++ b/gdb/testsuite/gdb.ada/catch_ex_std.exp > @@ -13,12 +13,10 @@ > # You should have received a copy of the GNU General Public License > # along with this program. If not, see . > > -require allow_shlib_tests > +require allow_shlib_tests allow_ada_tests > > load_lib "ada.exp" > > -require allow_ada_tests > - > standard_ada_testfile foo > > set srcfile2 [file join [file dirname $srcfile] some_package.adb] > diff --git a/gdb/testsuite/gdb.ada/dynamic-iface.exp b/gdb/testsuite/gdb.ada/dynamic-iface.exp > index 431fa29d64f..b523968194a 100644 > --- a/gdb/testsuite/gdb.ada/dynamic-iface.exp > +++ b/gdb/testsuite/gdb.ada/dynamic-iface.exp > @@ -15,9 +15,7 @@ > > load_lib "ada.exp" > > -require allow_ada_tests > - > -require gnat_runtime_has_debug_info > +require allow_ada_tests gnat_runtime_has_debug_info > > standard_ada_testfile main > > diff --git a/gdb/testsuite/gdb.ada/exec_changed.exp b/gdb/testsuite/gdb.ada/exec_changed.exp > index 9e69d136e31..087d1ac9b3d 100644 > --- a/gdb/testsuite/gdb.ada/exec_changed.exp > +++ b/gdb/testsuite/gdb.ada/exec_changed.exp > @@ -15,11 +15,9 @@ > > load_lib "ada.exp" > > -require allow_ada_tests > - > # This testcase verifies the behavior of the `start' command, which > # does not work when we use the gdb stub... > -require !use_gdb_stub > +require allow_ada_tests !use_gdb_stub > > standard_ada_testfile first > --- a/gdb/testsuite/gdb.ada/start.exp > +++ b/gdb/testsuite/gdb.ada/start.exp > @@ -15,11 +15,9 @@ > > load_lib "ada.exp" > > -require allow_ada_tests > - > # This testcase verifies the behavior of the `start' command, which > # does not work when we use the gdb stub... > -require !use_gdb_stub > +require allow_ada_tests !use_gdb_stub > ... > standard_ada_testfile dummy > > diff --git a/gdb/testsuite/gdb.ada/tagged.exp b/gdb/testsuite/gdb.ada/tagged.exp > index f45a6509da5..a842ed2b6ba 100644 > index fb56d2602b1..970f44da21c 100644 > --- a/gdb/testsuite/gdb.base/async-shell.exp > +++ b/gdb/testsuite/gdb.base/async-shell.exp > @@ -15,10 +15,8 @@ > > standard_testfile > > -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 I'm not really sure consolidation when we have a comment describing each "require" line before is an improvement. For example, above, it was clear before that the use_gdb_stub requirement was related to "run". Afterwards, it is not clear whether the support_displaced_stepping check is related to the comment. > > --- a/gdb/testsuite/gdb.base/fork-print-inferior-events.exp > +++ b/gdb/testsuite/gdb.base/fork-print-inferior-events.exp > @@ -20,11 +20,9 @@ > # 'set detach-on-fork [on,off]' are the correct ones. > > # 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 > +require !use_gdb_stub !gdb_debug_enabled > Here similarly. Having a require line connected to the comment as before seemed better to me, as it's more localized, it's obvious to which require expression the comment is refering to. If we always merge, then I fear that connection will be lost in some less obvious cases. > diff --git a/gdb/testsuite/gdb.base/solib-display.exp b/gdb/testsuite/gdb.base/solib-display.exp > index aaaa134007b..913203ecfb9 100644 > --- a/gdb/testsuite/gdb.base/solib-display.exp > +++ b/gdb/testsuite/gdb.base/solib-display.exp > @@ -28,8 +28,6 @@ > # (and thus aren't affected by shared library unloading) are not > # disabled prematurely. > > -require allow_shlib_tests > - > # This test is currently not supported for stub targets, because it uses the > # start command (through gdb_start_cmd). In theory, it could be changed to > # use something else (kill + gdb_run_cmd with a manual breakpoint at main). > @@ -43,7 +41,7 @@ require allow_shlib_tests > # This is because the initial stop is done before the shared libraries are > # loaded. > > -require !use_gdb_stub > +require allow_shlib_tests !use_gdb_stub > Here too. Etc. > # Library file. > set libname "solib-display-lib" > diff --git a/gdb/testsuite/gdb.base/solib-nodir.exp b/gdb/testsuite/gdb.base/solib-nodir.exp > index dd0724909ad..f5d94798f9d 100644 > --- a/gdb/testsuite/gdb.base/solib-nodir.exp > +++ b/gdb/testsuite/gdb.base/solib-nodir.exp > @@ -13,18 +13,16 @@ > # You should have received a copy of the GNU General Public License > # along with this program. If not, see . */ > > -require allow_shlib_tests > +# We need to be able to influence the target's environment and working > +# directory. Can't do that if when we connect the inferior is already > +# running. > +require allow_shlib_tests !use_gdb_stub > > # The testcase assumes the target can access the OBJDIR. > if [is_remote target] { > return > } > > -# We need to be able to influence the target's environment and working > -# directory. Can't do that if when we connect the inferior is already > -# running. > -require !use_gdb_stub Ditto. The connection between "already running" and use_gdb_stub ends up diluted. Same for the rest of patch where we have comments for each require.