| Message ID | 57FF41AB.4020301@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
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 <kyrylo.tkachov@arm.com> > > * check_GNU_style.sh (remove_testsuite): New function. > Use it to remove testsuite from the diff. > >> Martin >
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 <kyrylo.tkachov@arm.com> >> >> * check_GNU_style.sh (remove_testsuite): New function. >> Use it to remove testsuite from the diff. >> >>> Martin >> >
On Oct 21, 2016, at 12:47 PM, Martin Sebor <msebor@gmail.com> 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.
On 10/21/2016 10:59 PM, Mike Stump wrote: > On Oct 21, 2016, at 12:47 PM, Martin Sebor <msebor@gmail.com> 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
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 <msebor@gmail.com> 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 >
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), \