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

[Tom de Vries <tdevries@suse.de>]

[-- 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