develooper Front page | perl.perl5.porters | Postings from November 2017

Re: $EXPORT_ALL now warns

Thread Previous
From:
demerphq
Date:
November 17, 2017 09:58
Subject:
Re: $EXPORT_ALL now warns
Message ID:
CANgJU+VkmJr3TqR50wqsPTDDYuO9T7x_o7GmcVpAzZwLajMehA@mail.gmail.com
Since I was on my phone when I wrote my original reply, I wanted to
reply in more detail.

On 17 November 2017 at 09:45, demerphq <demerphq@gmail.com> wrote:
> On 17 Nov 2017 05:11, "Father Chrysostomos" <sprout@cpan.org> wrote:
>
> $EXPORT_ALL now warns
>
> Yves Orton made this commit about a year ago:
>> commit eafd371ce091bf61fe1673448eee5e53c4b0ef32
>> Author: Yves Orton <demerphq@gmail.com>
>> Date:   Wed Oct 19 11:09:00 2016 +0200
>>
>>     gv.c: rework special var matching logic to be simpler
>>
>>     The old code used a bunch of tricks that IMO are unlikely
>>     to actually make any performance difference, and make
>>     the code harder to read, search, and understand, and less
>>     amenable to using string constant friendly macros.
>>
>>     So for instance instead of switching on the first char
>>     and then comparing the second on for an exact match
>>     instead just compare the full string. The extra char
>>     is unlikely to make a difference to the underlying code
>>     that gets called, and the end result is easier searching.
>>
>>     Another issue is that some of the code was inadverdantly
>>     doing prefix matches, and not exact matchs for certain
>>     magic var names. This is fixed as part of these changes.
>>
>> I would argu that the EXPORT check was not inadvertently a prefix
>> check.  And when somebody proposed changing it in another thread a
>> few years back, I cited various CPAN modules that showed that people
>> were relying on it.

 I think that the evidence is ambiguous on this at best. (Without
asking Larry which I have not done.)

This code was part of the perl 5 mega-commit a0d0e21e.

There is no comment to explain why it uses strneq() and not streq().
There is nothing to say that it is deliberately exempting any var of
any type that starts with EXPORTER. It is just as reasonable to say
its just a simple way to allow the various intended EXPORT vars
without realizing the full implications.

There is no documentation or comment to support that every package var
of every type that starts with EXPORT is exempt from use once
warnings.

We document that @EXPORT, @EXPORT_OK and @EXPORT_ALL and %EXPORT_TAGS
are special to perl (and thus implicitly explain why they are exempt).

So it seems as reasonable to assume that it was an oversight as it was
it was intentional.

Also, I disagree with the contention that they are "using" anything.
The bug here was that we did not warn when we should have. They may be
benefiting from this oversight, but since we did not document or
promise it, I contend this is a bugfix restoring promised behavior,
not a regression of removing one that we have committed to providing.

> We discussed it as the time and decided that since it meant
>
> @EXPORT_ALLL
>
> and similar typos would be silently ignored and that the behaviour was
> undocumented this change was warranted.

To restate this point in different words: it seems that the
consequences of ignoring typos of these variables is worse than
restoring these warnings.

> In any case, I recommend we revert to the previous behaviour of
> exempting anything beginning with EXPORT from 'used once' warn-
> ings.  And the change should be backported to 5.26.x, since it is a
> regression.
>
>
> I don't think that is the right thing to do frankly.

In other words, we have other options. For instance a patch like this:

diff --git a/gv.c b/gv.c
index bc5b388..1d19de6 100644
--- a/gv.c
+++ b/gv.c
@@ -1903,7 +1903,8 @@ S_gv_magicalize(pTHX_ GV *gv, HV *stash, const
char *name, STRLEN len,
                     (memEQs(name, len, "EXPORT")
                     ||memEQs(name, len, "EXPORT_OK")
                     ||memEQs(name, len, "EXPORT_FAIL")
-                    ||memEQs(name, len, "EXPORT_TAGS"))
+                    ||memEQs(name, len, "EXPORT_TAGS")
+                    ||memEQs(name, len, "EXPORT_ANY"))
                 )
                    GvMULTI_on(gv);
                break;


But frankly IMO the right thing to do is add a way to register
specific variable names and types as being exempt from used once
warnings. Then we can get rid of all of this malarky: EXPORT isnt even
*really* part of core, it is an Exporter.pm thing whos interface got
blessed into semi-official status. Why shouldnt any module be able to
register *its* control variable as well? If we had that then this
problem would go away. If you used Exporter then @EXPORT_ALL would be
fine, if you didn't we would warn.

cheers,
Yves

Thread Previous


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About