Jkeenan/warnings t uni

[jkeenan]

This p.r. should bring t/uni/*.t into compliance with warnings-by-default. Though the overall diff is not that big (IMO), there were a lot of edge cases that had to be flushed out.

[jkeenan]

@xsawyerx asked:

1. With respect to changes in t/uni/universal.t:

   > Why these packages?

   To suppress warnings observed in this file in the target branch, alpha-dev-03-strict:

   $ cd t; TEST_JOBS=1 ./perl harness  uni/universal.t; cd -
   uni/universal.t .. Name "main::b" used only once: possible typo at uni/universal.t line 105.
   uni/universal.t .. 1/90 While trying to resolve method call splàtt->isa() can not locate package "zlòpp" yet it is mentioned in @splàtt::ISA (perhaps you forgot to load "zlòpp"?) at uni/universal.t line 153.
   While trying to resolve method call splàtt->isa() can not locate package "plòp" yet it is mentioned in @splàtt::ISA (perhaps you forgot to load "plòp"?) at uni/universal.t line 159.
   uni/universal.t .. ok     
   All tests successful.
   Files=1, Tests=90,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.01 cusr  0.00 csys =  0.02 CPU)
   Result: PASS

   The changes cited load the packages needed to suppress those warnings.

2. With respect to changes in t/uni/variables.t:

   > Why this comment?

   The left-open brace in the regex was causing my editor (vim) to mismatch opening and closing braces.

3. Again with respect to changes in t/uni/variables.t:

   > Why this entire test?

   Again, to suppress warnings observed in this file in the target branch, alpha-dev-03-strict:

   
   $ cd t; TEST_JOBS=1 ./perl harness  uni/variables.t; cd -
   uni/variables.t .. Scalar value @foo{"a"} better written as $foo{"a"} at uni/variables.t line 402.
   Scalar value @foo{"a"} better written as $foo{"a"} at uni/variables.t line 406.
   Name "main::colon" used only once: possible typo at uni/variables.t line 33.
   uni/variables.t .. 1/66880 # Failed test 44 -   TODO   ... and doesn't generate any warnings at uni/variables.t line 244
   # Failed test 98 -   TODO   ... and doesn't generate any warnings at uni/variables.t line 244
   # Failed test 104 -   TODO   ... and doesn't generate any warnings at uni/variables.t line 244
   # Failed test 110 -   TODO   ... and doesn't generate any warnings at uni/variables.t line 244
   # Failed test 116 -   TODO   ... and doesn't generate any warnings at uni/variables.t line 244
   # Failed test 122 -   TODO   ... and doesn't generate any warnings at uni/variables.t line 244
   # Failed test 236 -   TODO   ... and doesn't generate any warnings at uni/variables.t line 244
   uni/variables.t .. 63827/66880 Array found where operator expected at (eval 66101) line 2, near "$$@;"
   (Missing operator before @;?)
   uni/variables.t .. Failed 7/66880 subtests 
   (less 515 skipped subtests: 66358 okay)

Test Summary Report

uni/variables.t (Wstat: 0 Tests: 66880 Failed: 7) Failed tests: 44, 98, 104, 110, 116, 122, 236 Files=1, Tests=66880, 3 wallclock secs ( 2.00 usr 0.07 sys + 2.71 cusr 0.06 csys = 4.84 CPU) Result: FAIL

It took quite some effort to identify the place in the file where this warning was being generated:

uni/variables.t .. 63827/66880 Array found where operator expected at (eval 66101) line 2, near "$$@;" (Missing operator before @;?)

... because the warning is coming from inside an `eval`.  It turned out to be coming from inside this `for` loop:

for my $chr ( q{@}, "\N{U+FF10}", "\N{U+0300}" ) {

But since the warning was only being thrown **once** within the loop, that meant that 2 iterations of the loop were **not** generating a warning, which implied that the warning only needed to be suppressed on 1 iteration.  Experimentation demonstrated that the warning was being thrown by `q{@}`.

In general, with respect either to relaxing strictures or suppressing warnings, our approach has been to apply a change in the narrowest scope that is both possible and feasible.  So in this case, we pull `q{@}` out into a block of its own and `no warnings 'syntax';` there.  We leave the other two instances as they were.

**Note:** I'm consolidating your three questions into one response so that we only have to follow one email thread per issue or pull request.  For more on this approach, please see our [wiki page](https://github.com/atoomic/perl/wiki/How-to-Start-Working-with-Tests-in-the-Perl-Core-Distribution#your-mission-should-you-choose-to-accept-it).

Thank you very much.
Jim Keenan

[xsawyerx]

Thank you for the detailed response, Jim. I forgot to ask another question: Why did you pull in the perldiag message?

[jkeenan]

Thank you for the detailed response, Jim. I forgot to ask another question: Why did you pull in the perldiag message?

In the course of working on this proof-of-concept we have encountered situations where something is wrong or, at least, sub-optimal in the Perl 5 core distribution as of the point in time (tag v5.32.0) at which we forked off and began our work. Such situations may be "out of scope" for whatever our objective is at the time (e.g., version bump; strict-by-default; warnings-by-default) -- but at the same time a correction would be desirable.

So our approach has been to open tickets in https://github.com/Perl/perl5/issues. You can find some of these issues by looking for the "discovered-thru-p7-research" label. These, of course, can lead to pull requests in the Perl 5 repository.

Some of these issues/p.r.s are accepted by Perl 5; some are not. If a p.r. is accepted, then why not cherry-pick it into our repo so that we get the benefit.

That was the case here. See: https://github.com/Perl/perl5/issues/17935 https://github.com/Perl/perl5/pull/17936

[jkeenan]

@xsawyerx , other reviewers: Any further concerns? Otherwise, I intend to merge soon.

Thank you very much. Jim Keenan

[jkeenan]

@xsawyerx , other reviewers: Any further concerns? Otherwise, I intend to merge soon.

Thank you very much. Jim Keenan

No further comments made. Proceeding to merge.