| Message ID | 20210811220921.9B8E4203CF@pchp3.se.axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | gfortran.dg/PR82376.f90: Avoid matching a file-path. | expand |
On Thu, 12 Aug 2021 00:09:21 +0200 Hans-Peter Nilsson via Fortran <fortran@gcc.gnu.org> wrote: > I had a file-path to sources with the substring "new" in it, > and (only) this test regressed compared to results from > another build without "new" in the name. > > The test does > ! { dg-final { scan-tree-dump-times "new" 4 "original" } } > i.e. the contents of the tree-dump-file .original needs to match > the undelimited string "new" exactly four times. Very brittle. > > In the dump-file, there are three lines with calls to new: > D.908 = new ((integer(kind=4) *) data); > integer(kind=4) * new (integer(kind=4) & data) > static integer(kind=4) * new (integer(kind=4) &); > > But, there's also a line, which for me and cris-elf looked like: > _gfortran_runtime_error_at (&"At line 46 of file > /X/xyzzynewfrob/gcc/testsuite/gfortran.dg/PR82376.f90"[1]{lb: 1 sz: 1}, > &"Pointer actual argument \'new\' is not associated"[1]{lb: 1 sz: 1}); > The fourth match is obviously intended to match this line, but only > with *one* match, whereas the path can as above yield another hit. > > With Tcl, the regexp for matching the " " *and* the "'" > *and* the "\" gets a bit unsightly, so I suggest just > matching the "new" calls, which according to the comment in > the test is the key point. You can't have a file-path with > spaces and parentheses in a gcc build. I'm also making use > of {} rather than "" needing one level of quoting; the "\(" > is needed because the matched string is a regexp. > > Ok to commit? A wordmatch would be \mnew\M but i agree that counting calls by {\mnew (} is fine too. I'd call it obvious, so i dare to approve it. OK. thanks! > > testsuite: > * gfortran.dg/PR82376.f90: Robustify match. > --- > gcc/testsuite/gfortran.dg/PR82376.f90 | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/gcc/testsuite/gfortran.dg/PR82376.f90 b/gcc/testsuite/gfortran.dg/PR82376.f90 > index 07143ab7e82e..b99779ce9d8a 100644 > --- a/gcc/testsuite/gfortran.dg/PR82376.f90 > +++ b/gcc/testsuite/gfortran.dg/PR82376.f90 > @@ -2,7 +2,8 @@ > ! { dg-options "-fdump-tree-original -fcheck=pointer" } > ! > ! Test the fix for PR82376. The pointer check was doubling up the call > -! to new. The fix reduces the count of 'new' from 5 to 4. > +! to new. The fix reduces the count of 'new' from 5 to 4, or to 3, when > +! counting only calls. > ! > ! Contributed by José Rui Faustino de Sousa <jrfsousa@gmail.com> > ! > @@ -56,4 +57,4 @@ contains > end subroutine set > > end program main_p > -! { dg-final { scan-tree-dump-times "new" 4 "original" } } > +! { dg-final { scan-tree-dump-times { new \(} 3 "original" } }
> From: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> > Date: Thu, 12 Aug 2021 09:03:50 +0200 > On Thu, 12 Aug 2021 00:09:21 +0200 > Hans-Peter Nilsson via Fortran <fortran@gcc.gnu.org> wrote: > > > I had a file-path to sources with the substring "new" in it, > > and (only) this test regressed compared to results from > > another build without "new" in the name. > > > > The test does > > ! { dg-final { scan-tree-dump-times "new" 4 "original" } } > > i.e. the contents of the tree-dump-file .original needs to match > > the undelimited string "new" exactly four times. Very brittle. > > > > In the dump-file, there are three lines with calls to new: > > D.908 = new ((integer(kind=4) *) data); > > integer(kind=4) * new (integer(kind=4) & data) > > static integer(kind=4) * new (integer(kind=4) &); > > > > But, there's also a line, which for me and cris-elf looked like: > > _gfortran_runtime_error_at (&"At line 46 of file > > /X/xyzzynewfrob/gcc/testsuite/gfortran.dg/PR82376.f90"[1]{lb: 1 sz: 1}, > > &"Pointer actual argument \'new\' is not associated"[1]{lb: 1 sz: 1}); > > The fourth match is obviously intended to match this line, but only > > with *one* match, whereas the path can as above yield another hit. > > > > With Tcl, the regexp for matching the " " *and* the "'" > > *and* the "\" gets a bit unsightly, so I suggest just > > matching the "new" calls, which according to the comment in > > the test is the key point. You can't have a file-path with > > spaces and parentheses in a gcc build. I'm also making use > > of {} rather than "" needing one level of quoting; the "\(" > > is needed because the matched string is a regexp. > > > > Ok to commit? > > A wordmatch would be \mnew\M but i agree that counting calls by > {\mnew (} is fine too. Not really; I guess I should have mentioned that I briefly considered word-delimeters, but it'd match a subdirectory named "new"; not to be unexpected in gcc builds. Matching something that can't be in a file-path (of a gcc build) is the only way to be sure (that I can think of). > I'd call it obvious, so i dare to approve it. > OK. > thanks! Thanks, but not coming from a testsuite or fortran maintainer I'm not sure I can actually rely on that. OTOH, damn the torpedoes. Committed. > > > > testsuite: > > * gfortran.dg/PR82376.f90: Robustify match. > > --- > > gcc/testsuite/gfortran.dg/PR82376.f90 | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/gcc/testsuite/gfortran.dg/PR82376.f90 b/gcc/testsuite/gfortran.dg/PR82376.f90 > > index 07143ab7e82e..b99779ce9d8a 100644 > > --- a/gcc/testsuite/gfortran.dg/PR82376.f90 > > +++ b/gcc/testsuite/gfortran.dg/PR82376.f90 > > @@ -2,7 +2,8 @@ > > ! { dg-options "-fdump-tree-original -fcheck=pointer" } > > ! > > ! Test the fix for PR82376. The pointer check was doubling up the call > > -! to new. The fix reduces the count of 'new' from 5 to 4. > > +! to new. The fix reduces the count of 'new' from 5 to 4, or to 3, when > > +! counting only calls. > > ! > > ! Contributed by José Rui Faustino de Sousa <jrfsousa@gmail.com> > > ! > > @@ -56,4 +57,4 @@ contains > > end subroutine set > > > > end program main_p > > -! { dg-final { scan-tree-dump-times "new" 4 "original" } } > > +! { dg-final { scan-tree-dump-times { new \(} 3 "original" } } >
On 12.08.21 14:13, Hans-Peter Nilsson via Fortran wrote: >> I'd call it obvious, so i dare to approve it. >> OK. >> thanks! > Thanks, but not coming from a testsuite or fortran > maintainer I'm not sure I can actually rely on that. > > OTOH, damn the torpedoes. Committed. If it helps: A post-commit LGTM from my side. I think it can also be regarded as obvious.* Tobias *Albeit that reminds me of a professor in mathematics who told us that he tends to very carefully check those places where the (course, bachelor, master, PhD, ...) student writes: 'one then trivially obtains ...' ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
diff --git a/gcc/testsuite/gfortran.dg/PR82376.f90 b/gcc/testsuite/gfortran.dg/PR82376.f90 index 07143ab7e82e..b99779ce9d8a 100644 --- a/gcc/testsuite/gfortran.dg/PR82376.f90 +++ b/gcc/testsuite/gfortran.dg/PR82376.f90 @@ -2,7 +2,8 @@ ! { dg-options "-fdump-tree-original -fcheck=pointer" } ! ! Test the fix for PR82376. The pointer check was doubling up the call -! to new. The fix reduces the count of 'new' from 5 to 4. +! to new. The fix reduces the count of 'new' from 5 to 4, or to 3, when +! counting only calls. ! ! Contributed by José Rui Faustino de Sousa <jrfsousa@gmail.com> ! @@ -56,4 +57,4 @@ contains end subroutine set end program main_p -! { dg-final { scan-tree-dump-times "new" 4 "original" } } +! { dg-final { scan-tree-dump-times { new \(} 3 "original" } }
I had a file-path to sources with the substring "new" in it, and (only) this test regressed compared to results from another build without "new" in the name. The test does ! { dg-final { scan-tree-dump-times "new" 4 "original" } } i.e. the contents of the tree-dump-file .original needs to match the undelimited string "new" exactly four times. Very brittle. In the dump-file, there are three lines with calls to new: D.908 = new ((integer(kind=4) *) data); integer(kind=4) * new (integer(kind=4) & data) static integer(kind=4) * new (integer(kind=4) &); But, there's also a line, which for me and cris-elf looked like: _gfortran_runtime_error_at (&"At line 46 of file /X/xyzzynewfrob/gcc/testsuite/gfortran.dg/PR82376.f90"[1]{lb: 1 sz: 1}, &"Pointer actual argument \'new\' is not associated"[1]{lb: 1 sz: 1}); The fourth match is obviously intended to match this line, but only with *one* match, whereas the path can as above yield another hit. With Tcl, the regexp for matching the " " *and* the "'" *and* the "\" gets a bit unsightly, so I suggest just matching the "new" calls, which according to the comment in the test is the key point. You can't have a file-path with spaces and parentheses in a gcc build. I'm also making use of {} rather than "" needing one level of quoting; the "\(" is needed because the matched string is a regexp. Ok to commit? testsuite: * gfortran.dg/PR82376.f90: Robustify match. --- gcc/testsuite/gfortran.dg/PR82376.f90 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)