Re: [PATCH v2 79/79] Consolidate calls to require

[Pedro Alves <pedro@palves.net>]

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 <http://www.gnu.org/licenses/>.
>  
> -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 <http://www.gnu.org/licenses/>.  */
>  
> -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.