diff mbox series

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

Message ID 20230324114651.2987-1-tdevries@suse.de
State New
Headers
Series [RFC,gdb/testsuite] Add gdb/contrib/testsuite-normalize.sh |

Commit Message

Tom de Vries March 24, 2023, 11:46 a.m. UTC 
  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


base-commit: ca357a9359f5d09058d27fc1f84f1d53c2717ba1

Comments

Simon Marchi March 24, 2023, 1:52 p.m. UTC | #1 
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
Tom Tromey March 24, 2023, 2:16 p.m. UTC | #2 
>>>>> "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
Tom de Vries March 24, 2023, 2:39 p.m. UTC | #3 
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
Tom de Vries March 24, 2023, 2:42 p.m. UTC | #4 
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
diff mbox series

Patch

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 "$@"