[RFC] [gdb/testsuite] Add gdb/contrib/testsuite-normalize.sh

[RFC] [gdb/testsuite] Add gdb/contrib/testsuite-normalize.sh

[Tom de Vries]

There are a number of different ways to express the same thing in TCL.

For instance, $foo and ${foo}:
...
% set foo "foo bar"
foo bar
% puts ${foo}
foo bar
% puts $foo
foo bar
...

The braces make things (IMO) harder to read, but are necessary in some cases
though, for instance ${foo-bar} and $foo-bar are not the same thing:
...
% set foo "foo var"
foo var
% set foo-bar "foo-bar var"
foo-bar var
% puts ${foo-bar}
foo-bar var
% puts $foo-bar
foo var-bar
...

Furthermore, there's the tendency to use "$foo" (as is often necessary in shell
scripting), while in TCL using $foo is sufficient:
...
% set foo "bar bar"
bar bar
% puts "$foo"
bar bar
% puts $foo
bar bar
...

Add a script gdb/contrib/testsuite-normalize.sh, which rewrites test-cases
in a normal form, using $foo instead of ${foo}, and $foo instead of "$foo",
where possible.

This patch doesn't contain the effects of running it, because that would make
the patch too large:
...
$ git diff | wc
  63705  273146 2457187
...

If this approach is acceptable, I'll commit the effects as a seperate patch.

Tested on x86_64-linux.
---
 gdb/contrib/testsuite-normalize.sh | 44 ++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100755 gdb/contrib/testsuite-normalize.sh

diff --git a/gdb/contrib/testsuite-normalize.sh b/gdb/contrib/testsuite-normalize.sh
new file mode 100755
index 00000000000..0cbde3526ce
--- /dev/null
+++ b/gdb/contrib/testsuite-normalize.sh
@@ -0,0 +1,44 @@
+#!/bin/bash
+
+handle_file ()
+{
+    f="$1"
+
+    # Rewrite '${foo}' -> '$foo''.
+    # Don't rewrite '\${foo}' ->  '\$foo'.
+    # Don't rewrite '${foo}(bar)' -> '$foo(bar).
+    # Don't rewrite '${foo}bar' -> '$foobar'.
+    # Don't rewrite '${foo}::bar' -> '$foo::bar'.
+    # Two variants, first one matching '${foo}<eol>'.
+    sed -i 's/\([^\\]\)${\([a-zA-Z0-9_]*\)}$/\1$\2/g' "$f"
+    sed -i 's/\([^\\]\)${\([a-zA-Z0-9_]*\)}\([^a-zA-Z0-9_(:]\)/\1$\2\3/g' "$f"
+
+    # Rewrite ' "$foo" ' -> ' $foo '.
+    # Rewrite ' "$foo"<eol>' -> ' $foo<eol>'.
+    sed -i 's/\([ \t][ \t]*\)"\$\([a-zA-Z0-9_]*\)"$/\1$\2/g' "$f"
+    sed -i 's/\([ \t][ \t]*\)"\$\([a-zA-Z0-9_]*\)"\([ \t][ \t]*\)/\1$\2\3/g' "$f"
+}
+
+main ()
+{
+    if [ $# -eq 0 ]; then
+	mapfile files < <(find gdb/testsuite -type f -name "*.exp*")
+    else
+	files=("$@")
+    fi
+
+    for f in "${files[@]}"; do
+	f=$(echo $f)
+	sum=$(md5sum "$f")
+	while true; do
+            handle_file "$f"
+            prev=$sum
+            sum=$(md5sum "$f")
+            if [ "$sum" == "$prev" ]; then
+		break
+            fi
+	done
+    done
+}
+
+main "$@"

base-commit: ca357a9359f5d09058d27fc1f84f1d53c2717ba1
-- 
2.35.3

Re: [RFC] [gdb/testsuite] Add gdb/contrib/testsuite-normalize.sh

[Simon Marchi]

On 3/24/23 07:46, Tom de Vries via Gdb-patches wrote:
> There are a number of different ways to express the same thing in TCL.
> 
> For instance, $foo and ${foo}:
> ...
> % set foo "foo bar"
> foo bar
> % puts ${foo}
> foo bar
> % puts $foo
> foo bar
> ...
> 
> The braces make things (IMO) harder to read, but are necessary in some cases
> though, for instance ${foo-bar} and $foo-bar are not the same thing:
> ...
> % set foo "foo var"
> foo var
> % set foo-bar "foo-bar var"
> foo-bar var
> % puts ${foo-bar}
> foo-bar var
> % puts $foo-bar
> foo var-bar
> ...
> 
> Furthermore, there's the tendency to use "$foo" (as is often necessary in shell
> scripting), while in TCL using $foo is sufficient:
> ...
> % set foo "bar bar"
> bar bar
> % puts "$foo"
> bar bar
> % puts $foo
> bar bar
> ...
> 
> Add a script gdb/contrib/testsuite-normalize.sh, which rewrites test-cases
> in a normal form, using $foo instead of ${foo}, and $foo instead of "$foo",
> where possible.
> 
> This patch doesn't contain the effects of running it, because that would make
> the patch too large:
> ...
> $ git diff | wc
>   63705  273146 2457187
> ...
> 
> If this approach is acceptable, I'll commit the effects as a seperate patch.

I have nothing against it.  I like when the formatting is normalized
(which is why is <3 black for Python).

> Tested on x86_64-linux.

➜  gdb git:(master) ✗ pwd
/home/smarchi/src/binutils-gdb/gdb
➜  gdb git:(master) ✗ ./contrib/testsuite-normalize.sh
find: ‘gdb/testsuite’: No such file or directory

My CWD is the gdb directory 99% of the time.  IWBN if the script would
work regardless of where you are in the source repo (it looks like you
need to be in the root of the repo for it to work).  Perhaps it could
locate the root directory of the git repository and cd there?

I tried it and looked at a bunch of files, the changes seemed fine.

> +main ()
> +{
> +    if [ $# -eq 0 ]; then
> +	mapfile files < <(find gdb/testsuite -type f -name "*.exp*")
> +    else
> +	files=("$@")
> +    fi
> +
> +    for f in "${files[@]}"; do
> +	f=$(echo $f)

What's the purpose of this echo?  shellcheck says:


    In gdb/contrib/testsuite-normalize.sh line 31:
            f=$(echo $f)
              ^--------^ SC2116 (style): Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'.
                     ^-- SC2086 (info): Double quote to prevent globbing and word splitting.

    Did you mean:
            f=$(echo "$f")


Simon

Re: [RFC] [gdb/testsuite] Add gdb/contrib/testsuite-normalize.sh

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Furthermore, there's the tendency to use "$foo" (as is often necessary in shell
Tom> scripting), while in TCL using $foo is sufficient:

FWIW I sometimes find myself using unnecessary quotes in Tcl just to
make some things highlight differently in Emacs.  So I'd imagine there's
a lot more quoting that could be removed if we cared to.

Tom> Add a script gdb/contrib/testsuite-normalize.sh, which rewrites test-cases
Tom> in a normal form, using $foo instead of ${foo}, and $foo instead of "$foo",
Tom> where possible.

The idea seems fine to me.  I didn't really read the code.

Tom> +++ b/gdb/contrib/testsuite-normalize.sh
Tom> @@ -0,0 +1,44 @@
Tom> +#!/bin/bash
Tom> +

Should have a copyright header.

Tom

Re: [RFC] [gdb/testsuite] Add gdb/contrib/testsuite-normalize.sh

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 2958 bytes --]

On 3/24/23 14:52, Simon Marchi wrote:
> On 3/24/23 07:46, Tom de Vries via Gdb-patches wrote:
>> There are a number of different ways to express the same thing in TCL.
>>
>> For instance, $foo and ${foo}:
>> ...
>> % set foo "foo bar"
>> foo bar
>> % puts ${foo}
>> foo bar
>> % puts $foo
>> foo bar
>> ...
>>
>> The braces make things (IMO) harder to read, but are necessary in some cases
>> though, for instance ${foo-bar} and $foo-bar are not the same thing:
>> ...
>> % set foo "foo var"
>> foo var
>> % set foo-bar "foo-bar var"
>> foo-bar var
>> % puts ${foo-bar}
>> foo-bar var
>> % puts $foo-bar
>> foo var-bar
>> ...
>>
>> Furthermore, there's the tendency to use "$foo" (as is often necessary in shell
>> scripting), while in TCL using $foo is sufficient:
>> ...
>> % set foo "bar bar"
>> bar bar
>> % puts "$foo"
>> bar bar
>> % puts $foo
>> bar bar
>> ...
>>
>> Add a script gdb/contrib/testsuite-normalize.sh, which rewrites test-cases
>> in a normal form, using $foo instead of ${foo}, and $foo instead of "$foo",
>> where possible.
>>
>> This patch doesn't contain the effects of running it, because that would make
>> the patch too large:
>> ...
>> $ git diff | wc
>>    63705  273146 2457187
>> ...
>>
>> If this approach is acceptable, I'll commit the effects as a seperate patch.
> 
> I have nothing against it.  I like when the formatting is normalized
> (which is why is <3 black for Python).
> 

Yeah, I wish there was an existing tool we could use, but I haven't 
found anything.

>> Tested on x86_64-linux.
> 
> ➜  gdb git:(master) ✗ pwd
> /home/smarchi/src/binutils-gdb/gdb
> ➜  gdb git:(master) ✗ ./contrib/testsuite-normalize.sh
> find: ‘gdb/testsuite’: No such file or directory
> 
> My CWD is the gdb directory 99% of the time.  IWBN if the script would
> work regardless of where you are in the source repo (it looks like you
> need to be in the root of the repo for it to work).  Perhaps it could
> locate the root directory of the git repository and cd there?
> 

Ack, fixed (FWIW, not using cd though).

> I tried it and looked at a bunch of files, the changes seemed fine.
> 
>> +main ()
>> +{
>> +    if [ $# -eq 0 ]; then
>> +	mapfile files < <(find gdb/testsuite -type f -name "*.exp*")
>> +    else
>> +	files=("$@")
>> +    fi
>> +
>> +    for f in "${files[@]}"; do
>> +	f=$(echo $f)
> 
> What's the purpose of this echo?

Strip trailing newline.

>  shellcheck says:
> 
> 
>      In gdb/contrib/testsuite-normalize.sh line 31:
>              f=$(echo $f)
>                ^--------^ SC2116 (style): Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'.
>                       ^-- SC2086 (info): Double quote to prevent globbing and word splitting.
> 
>      Did you mean:
>              f=$(echo "$f")
> 
> 

I've managed to fix this by using instead:
...
        f=${f%$'\n'}
...

It passes shellcheck now.

Also added copyright notice, as requested by Tom in the other reply.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Add-gdb-contrib-testsuite-normalize.sh.patch --]
[-- Type: text/x-patch, Size: 3729 bytes --]

From e593407376bb55dbd8e5484098161f3cc6ffd4cf Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Sat, 4 Mar 2023 13:40:24 +0100
Subject: [PATCH] [gdb/testsuite] Add gdb/contrib/testsuite-normalize.sh

There are a number of different ways to express the same thing in TCL.

For instance, $foo and ${foo}:
...
% set foo "foo bar"
foo bar
% puts ${foo}
foo bar
% puts $foo
foo bar
...

The braces make things (IMO) harder to read, but are necessary in some cases
though, for instance ${foo-bar} and $foo-bar are not the same thing:
...
% set foo "foo var"
foo var
% set foo-bar "foo-bar var"
foo-bar var
% puts ${foo-bar}
foo-bar var
% puts $foo-bar
foo var-bar
...

Furthermore, there's the tendency to use "$foo" (as is often necessary in shell
scripting), while in TCL using $foo is sufficient:
...
% set foo "bar bar"
bar bar
% puts "$foo"
bar bar
% puts $foo
bar bar
...

Add a script gdb/contrib/testsuite-normalize.sh, which rewrites test-cases
in a normal form, using $foo instead of ${foo}, and $foo instead of "$foo",
where possible.

This patch doesn't contain the effects of running it, because that would make
the patch too large:
...
$ git diff | wc
  63705  273146 2457187
...

If this approach is acceptable, I'll commit the effects as a seperate patch.

Tested on x86_64-linux.
---
 gdb/contrib/testsuite-normalize.sh | 63 ++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100755 gdb/contrib/testsuite-normalize.sh

diff --git a/gdb/contrib/testsuite-normalize.sh b/gdb/contrib/testsuite-normalize.sh
new file mode 100755
index 00000000000..be5f89368ee
--- /dev/null
+++ b/gdb/contrib/testsuite-normalize.sh
@@ -0,0 +1,63 @@
+#!/usr/bin/env bash
+
+# Copyright (C) 2023 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+gdb_contrib_dir=$(cd "$(dirname "$0")" || exit; pwd -P)
+gdb_testsuite_dir=$gdb_contrib_dir/../testsuite
+
+handle_file ()
+{
+    f="$1"
+
+    # Rewrite '${foo}' -> '$foo''.
+    # Don't rewrite '\${foo}' ->  '\$foo'.
+    # Don't rewrite '${foo}(bar)' -> '$foo(bar).
+    # Don't rewrite '${foo}bar' -> '$foobar'.
+    # Don't rewrite '${foo}::bar' -> '$foo::bar'.
+    # Two variants, first one matching '${foo}<eol>'.
+    sed -i 's/\([^\\]\)${\([a-zA-Z0-9_]*\)}$/\1$\2/g' "$f"
+    sed -i 's/\([^\\]\)${\([a-zA-Z0-9_]*\)}\([^a-zA-Z0-9_(:]\)/\1$\2\3/g' "$f"
+
+    # Rewrite ' "$foo" ' -> ' $foo '.
+    # Rewrite ' "$foo"<eol>' -> ' $foo<eol>'.
+    sed -i 's/\([ \t][ \t]*\)"\$\([a-zA-Z0-9_]*\)"$/\1$\2/g' "$f"
+    sed -i 's/\([ \t][ \t]*\)"\$\([a-zA-Z0-9_]*\)"\([ \t][ \t]*\)/\1$\2\3/g' "$f"
+}
+
+main ()
+{
+    if [ $# -eq 0 ]; then
+	mapfile files < <(find "$gdb_testsuite_dir" -type f -name "*.exp*")
+    else
+	files=("$@")
+    fi
+
+    for f in "${files[@]}"; do
+	# Strip trailing newline.
+	f=${f%$'\n'}
+
+	sum=$(md5sum "$f")
+	while true; do
+            handle_file "$f"
+            prev=$sum
+            sum=$(md5sum "$f")
+            if [ "$sum" == "$prev" ]; then
+		break
+            fi
+	done
+    done
+}
+
+main "$@"

base-commit: 4460691252d5c345f0b34ac366639df23c687832
-- 
2.35.3

Re: [RFC] [gdb/testsuite] Add gdb/contrib/testsuite-normalize.sh

[Tom de Vries]

On 3/24/23 15:16, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> Furthermore, there's the tendency to use "$foo" (as is often necessary in shell
> Tom> scripting), while in TCL using $foo is sufficient:
> 
> FWIW I sometimes find myself using unnecessary quotes in Tcl just to
> make some things highlight differently in Emacs.  So I'd imagine there's
> a lot more quoting that could be removed if we cared to.
> 
> Tom> Add a script gdb/contrib/testsuite-normalize.sh, which rewrites test-cases
> Tom> in a normal form, using $foo instead of ${foo}, and $foo instead of "$foo",
> Tom> where possible.
> 
> The idea seems fine to me. 

Great, thanks.

> I didn't really read the code.
> 
> Tom> +++ b/gdb/contrib/testsuite-normalize.sh
> Tom> @@ -0,0 +1,44 @@
> Tom> +#!/bin/bash
> Tom> +
> 
> Should have a copyright header.

Ack, fixed in this ( 
https://sourceware.org/pipermail/gdb-patches/2023-March/198278.html ) 
update.

Thanks,
- Tom