Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Kyrill Tkachov]

On 24/10/16 12:01, Bernd Schmidt wrote:

On 10/21/2016 10:59 PM, Mike Stump wrote:

On Oct 21, 2016, at 12:47 PM, Martin Sebor  wrote:


The latest patch works as expected for me, both with an operand
and with stdin.  But since I'm not empowered to approve it one
of the others reviewers will need to give it their blessing.


Seems fine from a test suite perspective, but not my file.


Well, if it just takes someone to say "OK", I guess I'll do that. I don't awk 
however, so I'm relying on other people's judgement on that.


Thanks, I've committed it as r241471.
Please shout if you anyone sees anything dodgy with it.

Kyrill



Quite possibly the script should have testcases.


Bernd

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Bernd Schmidt]

On 10/21/2016 10:59 PM, Mike Stump wrote:

On Oct 21, 2016, at 12:47 PM, Martin Sebor  wrote:


The latest patch works as expected for me, both with an operand
and with stdin.  But since I'm not empowered to approve it one
of the others reviewers will need to give it their blessing.


Seems fine from a test suite perspective, but not my file.


Well, if it just takes someone to say "OK", I guess I'll do that. I 
don't awk however, so I'm relying on other people's judgement on that.


Quite possibly the script should have testcases.


Bernd

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Mike Stump]

On Oct 21, 2016, at 12:47 PM, Martin Sebor  wrote:
> 
> The latest patch works as expected for me, both with an operand
> and with stdin.  But since I'm not empowered to approve it one
> of the others reviewers will need to give it their blessing.

Seems fine from a test suite perspective, but not my file.

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Martin Sebor]

The latest patch works as expected for me, both with an operand
and with stdin.  But since I'm not empowered to approve it one
of the others reviewers will need to give it their blessing.

Thanks
Martin

On 10/21/2016 07:56 AM, Kyrill Tkachov wrote:

Ping.
https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00982.html

Thanks,
Kyrill

On 13/10/16 09:11, Kyrill Tkachov wrote:


On 12/10/16 17:49, Martin Sebor wrote:

On 10/12/2016 06:43 AM, Kyrill Tkachov wrote:


On 12/10/16 11:18, Kyrill Tkachov wrote:


On 12/10/16 10:57, Kyrill Tkachov wrote:


On 11/10/16 20:19, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 01:11:04PM -0600, Martin Sebor wrote:

Also, the pattern that starts with "/\+\+\+" looks like it's
missing
the ^ anchor.  Presumably it should be "/^\+\+\+ \/testsuite\//".

No, it will be almost never +++ /testsuite/
There needs to be .* in between "+++ " and "/testsuite/", and
perhaps
it should also ignore "+++ testsuite/".
So /^\+\+\+ (.*\/)?testsuite\// ?
Also, normally (when matching $0) there won't be newlines in the
text.

Jakub


Thanks.
Here is the updated patch with your suggestions.



Actually, I've encountered a problem:

 85 # Remove the testsuite part of the diff.  We don't care about GNU
style
 86 # in testcases and the dg-* directives give too many false
positives.
 87 remove_testsuite ()
 88 {
 89   awk 'BEGIN{testsuite=0} /\+\+\+ / && !
/testsuite\//{testsuite=0} \
 90{if (!testsuite) print} /^\+\+\+
(.*\/)?testsuite\//{testsuite=1}'
 91 }
 92
 93 grep $format '^+' $files \
 94 | remove_testsuite \
 95 | grep -v ':+++' \
 96 > $inp


The /^\+\+\+ (.*\/)?testsuite\// doesn't ever match when the ^ anchor
is used.
The awk command matches fine by itself but not when fed from the "grep
$format '^+' $files"
command because grep adds the line numbers and file names.
So is it okay to omit the ^ here?


I think the AWK regex will not work correctly when the patch has
the line number prefix like "1234: " (AFAICT, this can only happen
in the second invocation of the remove_testsuite function which
also has the problem below making me wonder if your testing
exercised that mode).



Huh, you're right, but it didn't cause problems in my testing, which
is weird.


I think the AWK regex needs to be changed to handle that.  It should
start with something like "^([1-9][0-9]*:)?\+\+\+"


I think it needs to be
^(.*:)?([1-9][0-9]*:)?\+\+\+
because grep -nH would add the filename as well as the line number in
the first
invocation of remove_testsuite.
This revision does that.



I tried to test the patch but it doesn't seem to work.  When passed
a patch as an argument it hangs.  The hunk below isn't quite right:

 # Don't reuse $inp, which may be generated using -H and thus
contain a
-# file prefix.
-grep -n '^+' $f \
+# file prefix.  Re-remove the testsuite since we're not using $inp.
+remove_testsuite $f \
+| grep -n '^+' \
 | grep -v ':+++' \
 > $tmp

The remove_testsuite function ignores arguments so passing $f to it
won't do anything except hang waiting for input.  This should look
closer to this (it worked in my very limited testing):

cat $f | remove_testsuite \



Thanks for the help,
Kyrill

2016-10-13  Kyrylo Tkachov  

* check_GNU_style.sh (remove_testsuite): New function.
Use it to remove testsuite from the diff.


Martin

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Kyrill Tkachov]

Ping.
https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00982.html

Thanks,
Kyrill

On 13/10/16 09:11, Kyrill Tkachov wrote:


On 12/10/16 17:49, Martin Sebor wrote:

On 10/12/2016 06:43 AM, Kyrill Tkachov wrote:


On 12/10/16 11:18, Kyrill Tkachov wrote:


On 12/10/16 10:57, Kyrill Tkachov wrote:


On 11/10/16 20:19, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 01:11:04PM -0600, Martin Sebor wrote:

Also, the pattern that starts with "/\+\+\+" looks like it's missing
the ^ anchor.  Presumably it should be "/^\+\+\+ \/testsuite\//".

No, it will be almost never +++ /testsuite/
There needs to be .* in between "+++ " and "/testsuite/", and perhaps
it should also ignore "+++ testsuite/".
So /^\+\+\+ (.*\/)?testsuite\// ?
Also, normally (when matching $0) there won't be newlines in the text.

Jakub


Thanks.
Here is the updated patch with your suggestions.



Actually, I've encountered a problem:

 85 # Remove the testsuite part of the diff.  We don't care about GNU
style
 86 # in testcases and the dg-* directives give too many false positives.
 87 remove_testsuite ()
 88 {
 89   awk 'BEGIN{testsuite=0} /\+\+\+ / && ! /testsuite\//{testsuite=0} \
 90{if (!testsuite) print} /^\+\+\+
(.*\/)?testsuite\//{testsuite=1}'
 91 }
 92
 93 grep $format '^+' $files \
 94 | remove_testsuite \
 95 | grep -v ':+++' \
 96 > $inp


The /^\+\+\+ (.*\/)?testsuite\// doesn't ever match when the ^ anchor
is used.
The awk command matches fine by itself but not when fed from the "grep
$format '^+' $files"
command because grep adds the line numbers and file names.
So is it okay to omit the ^ here?


I think the AWK regex will not work correctly when the patch has
the line number prefix like "1234: " (AFAICT, this can only happen
in the second invocation of the remove_testsuite function which
also has the problem below making me wonder if your testing
exercised that mode).



Huh, you're right, but it didn't cause problems in my testing, which is weird.


I think the AWK regex needs to be changed to handle that.  It should
start with something like "^([1-9][0-9]*:)?\+\+\+"


I think it needs to be
^(.*:)?([1-9][0-9]*:)?\+\+\+
because grep -nH would add the filename as well as the line number in the first
invocation of remove_testsuite.
This revision does that.



I tried to test the patch but it doesn't seem to work.  When passed
a patch as an argument it hangs.  The hunk below isn't quite right:

 # Don't reuse $inp, which may be generated using -H and thus contain a
-# file prefix.
-grep -n '^+' $f \
+# file prefix.  Re-remove the testsuite since we're not using $inp.
+remove_testsuite $f \
+| grep -n '^+' \
 | grep -v ':+++' \
 > $tmp

The remove_testsuite function ignores arguments so passing $f to it
won't do anything except hang waiting for input.  This should look
closer to this (it worked in my very limited testing):

cat $f | remove_testsuite \



Thanks for the help,
Kyrill

2016-10-13  Kyrylo Tkachov  

* check_GNU_style.sh (remove_testsuite): New function.
Use it to remove testsuite from the diff.


Martin

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Kyrill Tkachov]

On 12/10/16 17:49, Martin Sebor wrote:

On 10/12/2016 06:43 AM, Kyrill Tkachov wrote:


On 12/10/16 11:18, Kyrill Tkachov wrote:


On 12/10/16 10:57, Kyrill Tkachov wrote:


On 11/10/16 20:19, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 01:11:04PM -0600, Martin Sebor wrote:

Also, the pattern that starts with "/\+\+\+" looks like it's missing
the ^ anchor.  Presumably it should be "/^\+\+\+ \/testsuite\//".

No, it will be almost never +++ /testsuite/
There needs to be .* in between "+++ " and "/testsuite/", and perhaps
it should also ignore "+++ testsuite/".
So /^\+\+\+ (.*\/)?testsuite\// ?
Also, normally (when matching $0) there won't be newlines in the text.

Jakub


Thanks.
Here is the updated patch with your suggestions.



Actually, I've encountered a problem:

 85 # Remove the testsuite part of the diff.  We don't care about GNU
style
 86 # in testcases and the dg-* directives give too many false positives.
 87 remove_testsuite ()
 88 {
 89   awk 'BEGIN{testsuite=0} /\+\+\+ / && ! /testsuite\//{testsuite=0} \
 90{if (!testsuite) print} /^\+\+\+
(.*\/)?testsuite\//{testsuite=1}'
 91 }
 92
 93 grep $format '^+' $files \
 94 | remove_testsuite \
 95 | grep -v ':+++' \
 96 > $inp


The /^\+\+\+ (.*\/)?testsuite\// doesn't ever match when the ^ anchor
is used.
The awk command matches fine by itself but not when fed from the "grep
$format '^+' $files"
command because grep adds the line numbers and file names.
So is it okay to omit the ^ here?


I think the AWK regex will not work correctly when the patch has
the line number prefix like "1234: " (AFAICT, this can only happen
in the second invocation of the remove_testsuite function which
also has the problem below making me wonder if your testing
exercised that mode).



Huh, you're right, but it didn't cause problems in my testing, which is weird.


I think the AWK regex needs to be changed to handle that.  It should
start with something like "^([1-9][0-9]*:)?\+\+\+"


I think it needs to be
^(.*:)?([1-9][0-9]*:)?\+\+\+
because grep -nH would add the filename as well as the line number in the first
invocation of remove_testsuite.
This revision does that.



I tried to test the patch but it doesn't seem to work.  When passed
a patch as an argument it hangs.  The hunk below isn't quite right:

 # Don't reuse $inp, which may be generated using -H and thus contain a
-# file prefix.
-grep -n '^+' $f \
+# file prefix.  Re-remove the testsuite since we're not using $inp.
+remove_testsuite $f \
+| grep -n '^+' \
 | grep -v ':+++' \
 > $tmp

The remove_testsuite function ignores arguments so passing $f to it
won't do anything except hang waiting for input.  This should look
closer to this (it worked in my very limited testing):

cat $f | remove_testsuite \



Thanks for the help,
Kyrill

2016-10-13  Kyrylo Tkachov  

* check_GNU_style.sh (remove_testsuite): New function.
Use it to remove testsuite from the diff.


Martin


diff --git a/contrib/check_GNU_style.sh b/contrib/check_GNU_style.sh
index 87a276c9cf47b5e07c4407f740ce05dce1928c30..fb7494661ee8ff4d4e58ed05137bb69aab7c46a7 100755
--- a/contrib/check_GNU_style.sh
+++ b/contrib/check_GNU_style.sh
@@ -81,7 +81,17 @@ if [ $nfiles -eq 1 ]; then
 else
 format="-nH"
 fi
+
+# Remove the testsuite part of the diff.  We don't care about GNU style
+# in testcases and the dg-* directives give too many false positives.
+remove_testsuite ()
+{
+  awk 'BEGIN{testsuite=0} /^(.*:)?([1-9][0-9]*:)?\+\+\+ / && ! /testsuite\//{testsuite=0} \
+   {if (!testsuite) print} /^(.*:)?([1-9][0-9]*:)?\+\+\+ (.*\/)?testsuite\//{testsuite=1}'
+}
+
 grep $format '^+' $files \
+| remove_testsuite \
 | grep -v ':+++' \
 > $inp
 
@@ -160,8 +170,9 @@ col (){
 	fi
 
 	# Don't reuse $inp, which may be generated using -H and thus contain a
-	# file prefix.
-	grep -n '^+' $f \
+	# file prefix.  Re-remove the testsuite since we're not using $inp.
+	cat $f | remove_testsuite \
+	| grep -n '^+' \
 	| grep -v ':+++' \
 	> $tmp
 
@@ -174,11 +185,10 @@ col (){
 	# Expand tabs to spaces according to tab positions.
 	# Keep long lines, make short lines empty.  Print the part past 80 chars
 	# in red.
-# Don't complain about dg-xxx directives in tests.
 	cat "$tmp" \
 	| sed 's/^[0-9]*:+//' \
 	| expand \
-	| awk '$0 !~ /{[[:space:]]*dg-(error|warning|message)[[:space:]]/ { \
+	| awk '{ \
 		 if (length($0) > 80) \
 		   printf "%s\033[1;31m%s\033[0m\n", \
 			  substr($0,1,80), \

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Martin Sebor]

On 10/12/2016 06:43 AM, Kyrill Tkachov wrote:


On 12/10/16 11:18, Kyrill Tkachov wrote:


On 12/10/16 10:57, Kyrill Tkachov wrote:


On 11/10/16 20:19, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 01:11:04PM -0600, Martin Sebor wrote:

Also, the pattern that starts with "/\+\+\+" looks like it's missing
the ^ anchor.  Presumably it should be "/^\+\+\+ \/testsuite\//".

No, it will be almost never +++ /testsuite/
There needs to be .* in between "+++ " and "/testsuite/", and perhaps
it should also ignore "+++ testsuite/".
So /^\+\+\+ (.*\/)?testsuite\// ?
Also, normally (when matching $0) there won't be newlines in the text.

Jakub


Thanks.
Here is the updated patch with your suggestions.



Actually, I've encountered a problem:

 85 # Remove the testsuite part of the diff.  We don't care about GNU
style
 86 # in testcases and the dg-* directives give too many false positives.
 87 remove_testsuite ()
 88 {
 89   awk 'BEGIN{testsuite=0} /\+\+\+ / && ! /testsuite\//{testsuite=0} \
 90{if (!testsuite) print} /^\+\+\+
(.*\/)?testsuite\//{testsuite=1}'
 91 }
 92
 93 grep $format '^+' $files \
 94 | remove_testsuite \
 95 | grep -v ':+++' \
 96 > $inp


The /^\+\+\+ (.*\/)?testsuite\// doesn't ever match when the ^ anchor
is used.
The awk command matches fine by itself but not when fed from the "grep
$format '^+' $files"
command because grep adds the line numbers and file names.
So is it okay to omit the ^ here?


I think the AWK regex will not work correctly when the patch has
the line number prefix like "1234: " (AFAICT, this can only happen
in the second invocation of the remove_testsuite function which
also has the problem below making me wonder if your testing
exercised that mode).

I think the AWK regex needs to be changed to handle that.  It should
start with something like "^([1-9][0-9]*:)?\+\+\+"

I tried to test the patch but it doesn't seem to work.  When passed
a patch as an argument it hangs.  The hunk below isn't quite right:

# Don't reuse $inp, which may be generated using -H and thus contain a
-   # file prefix.
-   grep -n '^+' $f \
+   # file prefix.  Re-remove the testsuite since we're not using $inp.
+   remove_testsuite $f \
+   | grep -n '^+' \
| grep -v ':+++' \
> $tmp

The remove_testsuite function ignores arguments so passing $f to it
won't do anything except hang waiting for input.  This should look
closer to this (it worked in my very limited testing):

cat $f | remove_testsuite \

Martin

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Kyrill Tkachov]

On 12/10/16 11:18, Kyrill Tkachov wrote:


On 12/10/16 10:57, Kyrill Tkachov wrote:


On 11/10/16 20:19, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 01:11:04PM -0600, Martin Sebor wrote:

Also, the pattern that starts with "/\+\+\+" looks like it's missing
the ^ anchor.  Presumably it should be "/^\+\+\+ \/testsuite\//".

No, it will be almost never +++ /testsuite/
There needs to be .* in between "+++ " and "/testsuite/", and perhaps
it should also ignore "+++ testsuite/".
So /^\+\+\+ (.*\/)?testsuite\// ?
Also, normally (when matching $0) there won't be newlines in the text.

Jakub


Thanks.
Here is the updated patch with your suggestions.



Actually, I've encountered a problem:

 85 # Remove the testsuite part of the diff.  We don't care about GNU style
 86 # in testcases and the dg-* directives give too many false positives.
 87 remove_testsuite ()
 88 {
 89   awk 'BEGIN{testsuite=0} /\+\+\+ / && ! /testsuite\//{testsuite=0} \
 90{if (!testsuite) print} /^\+\+\+ (.*\/)?testsuite\//{testsuite=1}'
 91 }
 92
 93 grep $format '^+' $files \
 94 | remove_testsuite \
 95 | grep -v ':+++' \
 96 > $inp


The /^\+\+\+ (.*\/)?testsuite\// doesn't ever match when the ^ anchor is used.
The awk command matches fine by itself but not when fed from the "grep $format '^+' 
$files"
command because grep adds the line numbers and file names.
So is it okay to omit the ^ here?



Here is the patch omitting the ^.
Thanks,
Kyrill

2016-10-12  Kyrylo Tkachov  

* check_GNU_style.sh (remove_testsuite): New function.
Use it to remove testsuite from the diff.


Thanks,
Kyrill



diff --git a/contrib/check_GNU_style.sh b/contrib/check_GNU_style.sh
index 87a276c9cf47b5e07c4407f740ce05dce1928c30..92ce2eee2067b067c002f60b4a81eb3a5fb98ec5 100755
--- a/contrib/check_GNU_style.sh
+++ b/contrib/check_GNU_style.sh
@@ -81,7 +81,17 @@ if [ $nfiles -eq 1 ]; then
 else
 format="-nH"
 fi
+
+# Remove the testsuite part of the diff.  We don't care about GNU style
+# in testcases and the dg-* directives give too many false positives.
+remove_testsuite ()
+{
+  awk 'BEGIN{testsuite=0} /\+\+\+ / && ! /testsuite\//{testsuite=0} \
+   {if (!testsuite) print} /\+\+\+ (.*\/)?testsuite\//{testsuite=1}'
+}
+
 grep $format '^+' $files \
+| remove_testsuite \
 | grep -v ':+++' \
 > $inp
 
@@ -160,8 +170,9 @@ col (){
 	fi
 
 	# Don't reuse $inp, which may be generated using -H and thus contain a
-	# file prefix.
-	grep -n '^+' $f \
+	# file prefix.  Re-remove the testsuite since we're not using $inp.
+	remove_testsuite $f \
+	| grep -n '^+' \
 	| grep -v ':+++' \
 	> $tmp
 
@@ -174,11 +185,10 @@ col (){
 	# Expand tabs to spaces according to tab positions.
 	# Keep long lines, make short lines empty.  Print the part past 80 chars
 	# in red.
-# Don't complain about dg-xxx directives in tests.
 	cat "$tmp" \
 	| sed 's/^[0-9]*:+//' \
 	| expand \
-	| awk '$0 !~ /{[[:space:]]*dg-(error|warning|message)[[:space:]]/ { \
+	| awk '{ \
 		 if (length($0) > 80) \
 		   printf "%s\033[1;31m%s\033[0m\n", \
 			  substr($0,1,80), \

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Kyrill Tkachov]

On 12/10/16 10:57, Kyrill Tkachov wrote:


On 11/10/16 20:19, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 01:11:04PM -0600, Martin Sebor wrote:

Also, the pattern that starts with "/\+\+\+" looks like it's missing
the ^ anchor.  Presumably it should be "/^\+\+\+ \/testsuite\//".

No, it will be almost never +++ /testsuite/
There needs to be .* in between "+++ " and "/testsuite/", and perhaps
it should also ignore "+++ testsuite/".
So /^\+\+\+ (.*\/)?testsuite\// ?
Also, normally (when matching $0) there won't be newlines in the text.

Jakub


Thanks.
Here is the updated patch with your suggestions.



Actually, I've encountered a problem:

 85 # Remove the testsuite part of the diff.  We don't care about GNU style
 86 # in testcases and the dg-* directives give too many false positives.
 87 remove_testsuite ()
 88 {
 89   awk 'BEGIN{testsuite=0} /\+\+\+ / && ! /testsuite\//{testsuite=0} \
 90{if (!testsuite) print} /^\+\+\+ (.*\/)?testsuite\//{testsuite=1}'
 91 }
 92
 93 grep $format '^+' $files \
 94 | remove_testsuite \
 95 | grep -v ':+++' \
 96 > $inp


The /^\+\+\+ (.*\/)?testsuite\// doesn't ever match when the ^ anchor is used.
The awk command matches fine by itself but not when fed from the "grep $format '^+' 
$files"
command because grep adds the line numbers and file names.
So is it okay to omit the ^ here?

Thanks,
Kyrill

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Kyrill Tkachov]

On 11/10/16 20:19, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 01:11:04PM -0600, Martin Sebor wrote:

Also, the pattern that starts with "/\+\+\+" looks like it's missing
the ^ anchor.  Presumably it should be "/^\+\+\+ \/testsuite\//".

No, it will be almost never +++ /testsuite/
There needs to be .* in between "+++ " and "/testsuite/", and perhaps
it should also ignore "+++ testsuite/".
So /^\+\+\+ (.*\/)?testsuite\// ?
Also, normally (when matching $0) there won't be newlines in the text.

Jakub


Thanks.
Here is the updated patch with your suggestions.

2016-10-12  Kyrylo Tkachov  

* check_GNU_style.sh (remove_testsuite): New function.
Use it to remove testsuite from the diff.
commit e4af5426c86a00a05fc08109fa84bfe30285f2a2
Author: Kyrylo Tkachov 
Date:   Tue Oct 11 11:33:50 2016 +0100

[check_GNU_style.sh] Ignore testsuite

diff --git a/contrib/check_GNU_style.sh b/contrib/check_GNU_style.sh
index 87a276c..2787e9b 100755
--- a/contrib/check_GNU_style.sh
+++ b/contrib/check_GNU_style.sh
@@ -81,7 +81,17 @@ if [ $nfiles -eq 1 ]; then
 else
 format="-nH"
 fi
+
+# Remove the testsuite part of the diff.  We don't care about GNU style
+# in testcases and the dg-* directives give too many false positives.
+remove_testsuite ()
+{
+  awk 'BEGIN{testsuite=0} /^\+\+\+ / && ! /testsuite\//{testsuite=0} \
+   {if (!testsuite) print} /^\+\+\+ (.*\/)?testsuite\//{testsuite=1}'
+}
+
 grep $format '^+' $files \
+| remove_testsuite \
 | grep -v ':+++' \
 > $inp
 
@@ -160,8 +170,9 @@ col (){
 	fi
 
 	# Don't reuse $inp, which may be generated using -H and thus contain a
-	# file prefix.
-	grep -n '^+' $f \
+	# file prefix.  Re-remove the testsuite since we're not using $inp.
+	remove_testsuite $f \
+	| grep -n '^+' \
 	| grep -v ':+++' \
 	> $tmp
 
@@ -174,11 +185,10 @@ col (){
 	# Expand tabs to spaces according to tab positions.
 	# Keep long lines, make short lines empty.  Print the part past 80 chars
 	# in red.
-# Don't complain about dg-xxx directives in tests.
 	cat "$tmp" \
 	| sed 's/^[0-9]*:+//' \
 	| expand \
-	| awk '$0 !~ /{[[:space:]]*dg-(error|warning|message)[[:space:]]/ { \
+	| awk '{ \
 		 if (length($0) > 80) \
 		   printf "%s\033[1;31m%s\033[0m\n", \
 			  substr($0,1,80), \

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Jakub Jelinek]

On Tue, Oct 11, 2016 at 01:11:04PM -0600, Martin Sebor wrote:
> Also, the pattern that starts with "/\+\+\+" looks like it's missing
> the ^ anchor.  Presumably it should be "/^\+\+\+ \/testsuite\//".

No, it will be almost never +++ /testsuite/
There needs to be .* in between "+++ " and "/testsuite/", and perhaps
it should also ignore "+++ testsuite/".
So /^\+\+\+ (.*\/)?testsuite\// ?
Also, normally (when matching $0) there won't be newlines in the text.

Jakub

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Martin Sebor]

On 10/11/2016 09:22 AM, Kyrill Tkachov wrote:


On 11/10/16 16:13, Jeff Law wrote:

On 10/11/2016 05:01 AM, Bernd Schmidt wrote:

On 10/11/2016 12:56 PM, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 11:47:21AM +0100, Kyrill Tkachov wrote:

check_GNU_style.sh complains a lot about dg-* directives in the
testsuite and in particular about line lengths.
There's nothing we can do about the directives and sometimes they're
supposed to be long, in particular the scan-assembler
checks in dg-final.  Currently check_GNU_style.sh has code to avoid
warning for dg-* directives but it's too weak, it doesn't
catch dg-final or dg-options directives.
This patch makes the code ignore all "{ dg-.*" lines for length
purposes.
This eliminates a lot of false positives in my patches and didn't
filter any legitimate warnings in my patches.


I wonder if we just shouldn't ignore all line lengths in testcases (or
perhaps any coding style whatsoever).  Some testcases are meant to be
formatted more-less according to our coding style (but, e.g.
/* PR tree-optimization/12345 */
comments never end with . and 2 spaces), except for dg- directives, but
others are entered in whatever form they came from the reporter.


I agree, probably best not to check testcases for style (but then I
don't trust automatic checkers anyway).

Agreed that checking testcases for style isn't desirable.



Ok, here's a patch that does that by filtering out everything between
"+++.*testsuite/" and "+++".
Is this is an ok approach?
This eliminates all testsuite warnings on a few of my patches that I
tried it out on.

Disclaimer: I'm not an awk expert.


I've been meaning to propose a similar change for some time so thanks!
Just a couple of minor comments (nits really) about the AWK script.

+# Remove the testsuite part of the diff.  We don't care about GNU style
+# in testcases and the dg-* directives give too many false positives.
+remove_testsuite ()
+{
+  awk 'BEGIN{testsuite=0} /^\+\+\+[[:space:]]/ && ! 
/testsuite/{testsuite=0} \
+   {if (!testsuite) print} 
/\+\+\+[[:space:]].*testsuite\//{testsuite=1}'

+}

In the standard (POSIX) diff format, modified file names are prefixed
by a line matching the "^+++ [^ /]" regular expression (i.e., with
exactly one space after +++).  [[:space:]] matches other whitespace
characters including newline and carriage return in the POSIX locale,
and possibly other characters in named locales.  Using a literal space
in the regex should be safer.

Also, the pattern that starts with "/\+\+\+" looks like it's missing
the ^ anchor.  Presumably it should be "/^\+\+\+ \/testsuite\//".

Martin

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Kyrill Tkachov]

On 11/10/16 16:13, Jeff Law wrote:

On 10/11/2016 05:01 AM, Bernd Schmidt wrote:

On 10/11/2016 12:56 PM, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 11:47:21AM +0100, Kyrill Tkachov wrote:

check_GNU_style.sh complains a lot about dg-* directives in the
testsuite and in particular about line lengths.
There's nothing we can do about the directives and sometimes they're
supposed to be long, in particular the scan-assembler
checks in dg-final.  Currently check_GNU_style.sh has code to avoid
warning for dg-* directives but it's too weak, it doesn't
catch dg-final or dg-options directives.
This patch makes the code ignore all "{ dg-.*" lines for length
purposes.
This eliminates a lot of false positives in my patches and didn't
filter any legitimate warnings in my patches.


I wonder if we just shouldn't ignore all line lengths in testcases (or
perhaps any coding style whatsoever).  Some testcases are meant to be
formatted more-less according to our coding style (but, e.g.
/* PR tree-optimization/12345 */
comments never end with . and 2 spaces), except for dg- directives, but
others are entered in whatever form they came from the reporter.


I agree, probably best not to check testcases for style (but then I
don't trust automatic checkers anyway).

Agreed that checking testcases for style isn't desirable.



Ok, here's a patch that does that by filtering out everything between "+++.*testsuite/" and 
"+++".
Is this is an ok approach?
This eliminates all testsuite warnings on a few of my patches that I tried it 
out on.

Disclaimer: I'm not an awk expert.

Thanks,
Kyrill

2016-10-11  Kyrylo Tkachov  

* check_GNU_style.sh (remove_testsuite): New function.
Use it to remove testsuite from the diff.



jeff


diff --git a/contrib/check_GNU_style.sh b/contrib/check_GNU_style.sh
index 87a276c9cf47b5e07c4407f740ce05dce1928c30..f290a83e20eed7920155b2258d6cc6f424fc0dc0 100755
--- a/contrib/check_GNU_style.sh
+++ b/contrib/check_GNU_style.sh
@@ -81,7 +81,17 @@ if [ $nfiles -eq 1 ]; then
 else
 format="-nH"
 fi
+
+# Remove the testsuite part of the diff.  We don't care about GNU style
+# in testcases and the dg-* directives give too many false positives.
+remove_testsuite ()
+{
+  awk 'BEGIN{testsuite=0} /^\+\+\+[[:space:]]/ && ! /testsuite/{testsuite=0} \
+   {if (!testsuite) print} /\+\+\+[[:space:]].*testsuite\//{testsuite=1}'
+}
+
 grep $format '^+' $files \
+| remove_testsuite \
 | grep -v ':+++' \
 > $inp
 
@@ -162,6 +172,7 @@ col (){
 	# Don't reuse $inp, which may be generated using -H and thus contain a
 	# file prefix.
 	grep -n '^+' $f \
+	| remove_testsuite \
 	| grep -v ':+++' \
 	> $tmp
 
@@ -178,7 +189,7 @@ col (){
 	cat "$tmp" \
 	| sed 's/^[0-9]*:+//' \
 	| expand \
-	| awk '$0 !~ /{[[:space:]]*dg-(error|warning|message)[[:space:]]/ { \
+	| awk '{ \
 		 if (length($0) > 80) \
 		   printf "%s\033[1;31m%s\033[0m\n", \
 			  substr($0,1,80), \

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Jeff Law]

On 10/11/2016 05:01 AM, Bernd Schmidt wrote:

On 10/11/2016 12:56 PM, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 11:47:21AM +0100, Kyrill Tkachov wrote:

check_GNU_style.sh complains a lot about dg-* directives in the
testsuite and in particular about line lengths.
There's nothing we can do about the directives and sometimes they're
supposed to be long, in particular the scan-assembler
checks in dg-final.  Currently check_GNU_style.sh has code to avoid
warning for dg-* directives but it's too weak, it doesn't
catch dg-final or dg-options directives.
This patch makes the code ignore all "{ dg-.*" lines for length
purposes.
This eliminates a lot of false positives in my patches and didn't
filter any legitimate warnings in my patches.


I wonder if we just shouldn't ignore all line lengths in testcases (or
perhaps any coding style whatsoever).  Some testcases are meant to be
formatted more-less according to our coding style (but, e.g.
/* PR tree-optimization/12345 */
comments never end with . and 2 spaces), except for dg- directives, but
others are entered in whatever form they came from the reporter.


I agree, probably best not to check testcases for style (but then I
don't trust automatic checkers anyway).

Agreed that checking testcases for style isn't desirable.


jeff

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Kyrill Tkachov]

On 11/10/16 11:56, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 11:47:21AM +0100, Kyrill Tkachov wrote:

check_GNU_style.sh complains a lot about dg-* directives in the testsuite and 
in particular about line lengths.
There's nothing we can do about the directives and sometimes they're supposed 
to be long, in particular the scan-assembler
checks in dg-final.  Currently check_GNU_style.sh has code to avoid warning for 
dg-* directives but it's too weak, it doesn't
catch dg-final or dg-options directives.
This patch makes the code ignore all "{ dg-.*" lines for length purposes.
This eliminates a lot of false positives in my patches and didn't filter any 
legitimate warnings in my patches.

I wonder if we just shouldn't ignore all line lengths in testcases (or
perhaps any coding style whatsoever).  Some testcases are meant to be
formatted more-less according to our coding style (but, e.g.
/* PR tree-optimization/12345 */
comments never end with . and 2 spaces), except for dg- directives, but
others are entered in whatever form they came from the reporter.


I would be up for ignoring the testsuite altogether in check_GNU_style.sh.
If there's consensus on this I can look into making that change.

Kyrill


Jakub

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Bernd Schmidt]

On 10/11/2016 12:56 PM, Jakub Jelinek wrote:

On Tue, Oct 11, 2016 at 11:47:21AM +0100, Kyrill Tkachov wrote:

check_GNU_style.sh complains a lot about dg-* directives in the testsuite and 
in particular about line lengths.
There's nothing we can do about the directives and sometimes they're supposed 
to be long, in particular the scan-assembler
checks in dg-final.  Currently check_GNU_style.sh has code to avoid warning for 
dg-* directives but it's too weak, it doesn't
catch dg-final or dg-options directives.
This patch makes the code ignore all "{ dg-.*" lines for length purposes.
This eliminates a lot of false positives in my patches and didn't filter any 
legitimate warnings in my patches.


I wonder if we just shouldn't ignore all line lengths in testcases (or
perhaps any coding style whatsoever).  Some testcases are meant to be
formatted more-less according to our coding style (but, e.g.
/* PR tree-optimization/12345 */
comments never end with . and 2 spaces), except for dg- directives, but
others are entered in whatever form they came from the reporter.


I agree, probably best not to check testcases for style (but then I 
don't trust automatic checkers anyway).



Bernd

Re: [PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Jakub Jelinek]

On Tue, Oct 11, 2016 at 11:47:21AM +0100, Kyrill Tkachov wrote:
> check_GNU_style.sh complains a lot about dg-* directives in the testsuite and 
> in particular about line lengths.
> There's nothing we can do about the directives and sometimes they're supposed 
> to be long, in particular the scan-assembler
> checks in dg-final.  Currently check_GNU_style.sh has code to avoid warning 
> for dg-* directives but it's too weak, it doesn't
> catch dg-final or dg-options directives.
> This patch makes the code ignore all "{ dg-.*" lines for length purposes.
> This eliminates a lot of false positives in my patches and didn't filter any 
> legitimate warnings in my patches.

I wonder if we just shouldn't ignore all line lengths in testcases (or
perhaps any coding style whatsoever).  Some testcases are meant to be
formatted more-less according to our coding style (but, e.g.
/* PR tree-optimization/12345 */
comments never end with . and 2 spaces), except for dg- directives, but
others are entered in whatever form they came from the reporter.

Jakub

[PATCH][check_GNU_style.sh] More aggressively ignore dg-xxx directives

[Kyrill Tkachov]

Hi all,

check_GNU_style.sh complains a lot about dg-* directives in the testsuite and 
in particular about line lengths.
There's nothing we can do about the directives and sometimes they're supposed 
to be long, in particular the scan-assembler
checks in dg-final.  Currently check_GNU_style.sh has code to avoid warning for 
dg-* directives but it's too weak, it doesn't
catch dg-final or dg-options directives.
This patch makes the code ignore all "{ dg-.*" lines for length purposes.
This eliminates a lot of false positives in my patches and didn't filter any 
legitimate warnings in my patches.

Ok for trunk?

Thanks,
Kyrill

2016-10-11  Kyrylo Tkachov  

* check_GNU_style.sh (col): Ignore dg-XXX directives for line length
complaints.
commit d0685da3487fc4cc614c64851185fed5c350bb7b
Author: Kyrylo Tkachov 
Date:   Tue Oct 11 11:33:50 2016 +0100

[check_GNU_style.sh] More aggressively ignore dg-xxx directives

diff --git a/contrib/check_GNU_style.sh b/contrib/check_GNU_style.sh
index 87a276c..ed4e07f 100755
--- a/contrib/check_GNU_style.sh
+++ b/contrib/check_GNU_style.sh
@@ -178,7 +178,7 @@ col (){
 	cat "$tmp" \
 	| sed 's/^[0-9]*:+//' \
 	| expand \
-	| awk '$0 !~ /{[[:space:]]*dg-(error|warning|message)[[:space:]]/ { \
+	| awk '$0 !~ /{[[:space:]]*dg-[^[:space:]]+[[:space:]]/ { \
 		 if (length($0) > 80) \
 		   printf "%s\033[1;31m%s\033[0m\n", \
 			  substr($0,1,80), \