Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doc change for 'our' implies large incompatible change #13938

Closed
p5pRT opened this issue Jun 19, 2014 · 11 comments
Closed

Doc change for 'our' implies large incompatible change #13938

p5pRT opened this issue Jun 19, 2014 · 11 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Jun 19, 2014

Migrated from rt.perl.org#122132 (status was 'resolved')

Searchable as RT122132$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 19, 2014

From john.l.henning@gmail.com

Created by john.l.henning@gmail.com

For the 'our' function, a doc change in 5.18 seems to imply a
large incompatible change, which is likely not what was meant.

"old" = prior to 5.18
"new" = 5.18 and beyond

  old​: "does not necessarily create a variable"
  new​: "only declares an alias"

  old​: "declares a global variable that"
  new​: "makes a lexical alias to"

From the above, this reader wondered if the text boiled down to​:

  - 5.16 'our' _might_ create your simple global variable
  - 5.18 'our' definitely won't

In particular, I wondered whether this case would change​:

  $ perl --version
  This is perl 5, version 12, subversion 4 (v5.12.4)

  $ perl -e 'use strict; use warnings; our $a1; my $a1;'
  "my" variable $a1 masks earlier declaration in same scope at -e line 1.
  $

and conversely​:

  $ perl -e 'use strict; use warnings; my $a1; our $a1;'
  "our" variable $a1 masks earlier declaration in same scope at -e line 1.
  $

If the 5.18 'our' never creates actual storage space for
variables, then it seemed - to this reader - that the 'my' in
the example above would be _required_ by 5.18.

Yet, it is clearly not required nor wanted in 5.12.

Surely 5.18 would not have made such an incompatible change
without flagging it in perl5180delta, where it is not.

Therefore, I suggest that the documentation change has
introduced confusion for the programmer who just wants a simple
way to declare globals.

It appears that the change was made at or near to here, as part
of a long thread.

From​: David Golden
Date​: July 5, 2012 17​:19
Subject​: Re​: [perl #113974] package NAMESPACE manpage comments
Message ID​: CAOeq1c_Rxg97c=M7i7MbtEE=
QmGo+mn3zvuxZDgf0K9MHNWm2A@​mail.gmail.com

In commit 66b3001, I've adapted that and tried to clarify
documentation for 'our'.

I suspect that David has improved an important case, but has
made things less clear for​:

  - Simple-hearted programmer
  - Wants straightforward mechanism to declare globals
  - Has procedure with a couple hundred lines + handful of
  subroutines
  - I.e. big enough to want a distinction between global and
  non-global variables,
  - Small enough to not be worth 'packages' and modules.

[For completeness​: alternatives considered​:
  - 'my' is probably the actual thing needed, but it
  doesn't _sound_ globalish.
  - 'use vars' is plainly marked "use of this pragma is
  discouraged"
]

Perl Info

Flags:
    category=docs
    severity=medium

Site configuration information for perl 5.12.4:

Configured by _mdnsresponder at Wed Jun 20 13:42:53 PDT 2012.

Summary of my perl5 (revision 5 version 12 subversion 4) configuration:

  Platform:
    osname=darwin, osvers=12.0, archname=darwin-thread-multi-2level
    uname='darwin b1026.apple.com 12.0 darwin kernel version 12.0.0: tue
may 15 23:31:29 pdt 2012; root:xnu-2050.6.70~1release_x86_64 x86_64 '
    config_args='-ds -e -Dprefix=/usr -Dccflags=-g  -pipe  -Dldflags=
-Dman3ext=3pm -Duseithreads -Duseshrplib -Dinc_version_list=none -Dcc=clang'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='clang', ccflags ='-arch i386 -arch x86_64 -g -pipe -fno-common
-DPERL_DARWIN -fno-strict-aliasing -fstack-protector -I/usr/local/include',
    optimize='-Os',
    cppflags='-g -pipe -fno-common -DPERL_DARWIN -fno-strict-aliasing
-fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.2.1 Compatible Apple Clang 4.0
(tags/Apple/clang-418.0.60)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='clang -mmacosx-version-min=10.8', ldflags ='-arch i386 -arch x86_64
-fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib
    libs=-ldbm -ldl -lm -lutil -lc
    perllibs=-ldl -lm -lutil -lc
    libc=, so=dylib, useshrplib=true, libperl=libperl.dylib
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=bundle, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags='-arch i386 -arch x86_64 -bundle -undefined
dynamic_lookup -L/usr/local/lib -fstack-protector'

Locally applied patches:
    /Library/Perl/Updates/<version> comes before system perl directories
    installprivlib and installarchlib points to the Updates directory


@INC for perl 5.12.4:
    /Library/Perl/5.12/darwin-thread-multi-2level
    /Library/Perl/5.12
    /Network/Library/Perl/5.12/darwin-thread-multi-2level
    /Network/Library/Perl/5.12
    /Library/Perl/Updates/5.12.4/darwin-thread-multi-2level
    /Library/Perl/Updates/5.12.4
    /System/Library/Perl/5.12/darwin-thread-multi-2level
    /System/Library/Perl/5.12
    /System/Library/Perl/Extras/5.12/darwin-thread-multi-2level
    /System/Library/Perl/Extras/5.12
    .


Environment for perl 5.12.4:
    DYLD_LIBRARY_PATH (unset)
    HOME=/Users/jhenning
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LC_ALL=C
    LC_LANG=C
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)

PATH=/Users/jhenning/spec/cpu2006/v1.2/bin:/Applications/Xcode.app/Contents/Developer/usr/bin:/Users/jhenning/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/opt/X11/bin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 19, 2014

From @ikegami

On Wed, Jun 18, 2014 at 11​:04 PM, John Henning <perlbug-followup@​perl.org>
wrote​:

Surely 5.18 would not have made such an incompatible change
without flagging it in perl5180delta, where it is not.

C<our> wasn't changed. The documentation was simply updated to reflect what
C<our> actually does.

$ perl -E'package ModA; our $x; package ModB; $ModA​::x = 123; $ModB​::x =
456; say $x'
123

Therefore, I suggest that the documentation change has

introduced confusion for the programmer who just wants a simple
way to declare globals.

Don't let an C<our> declaration span a package decleration, or use C<< use
vars >>.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 19, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 19, 2014

From @xdg

On Wed, Jun 18, 2014 at 11​:04 PM, John Henning
<perlbug-followup@​perl.org> wrote​:

Therefore, I suggest that the documentation change has
introduced confusion for the programmer who just wants a simple
way to declare globals.

Looking at my patch, I think the confusion may be that I removed the
term "global". And after skimming other docs, I didn't see anything
that talks clearly about the difference between lexical and package
variables and how the latter are always global and have storage
allocated on demand.

Would this amendment be helpful? →
https://gist.github.com/dagolden/1695755ac26a28a00f95

David

--
David Golden <xdg@​xdg.me> Twitter/IRC​: @​xdg

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2014

From john.l.henning@gmail.com

Would this amendment be helpful? →
https://gist.github.com/dagolden/1695755ac26a28a00f95

Hi David, thank you for responding to my request. You suggest​:

  ... allocate storage for that name within the current scope. (Package
  variable storage is allocated on demand.)

How about making the parenthetical into a full explanation of what is
happening, aimed at the user who I described last night.

See below
  -john


Package variable storage is allocated on demand. Therefore,
if you say​:

  1 use strict;
  2 use warnings;
  3 our $flag;
  4 ...
  5 $flag = something

then the effect is​:
  - storage for $flag is allocated at line 5, when $flag is
  auto-instantiated
  - line 3 gives the permission that allows this to happen
  - line 2 will cause warnings if you try to create new
  instances of something named $flag

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2014

From @xdg

On Thu, Jun 19, 2014 at 11​:57 AM, John Henning <john.l.henning@​gmail.com> wrote​:

How about making the parenthetical into a full explanation of what is
happening, aimed at the user who I described last night.

My view is that we probably shouldn't be talking about storage at all
-- that's the job of the Perl runtime. The focus for "our", "my",
"state" and "local" should be on scope and visibility.

--
David Golden <xdg@​xdg.me> Twitter/IRC​: @​xdg

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2014

From @ikegami

On Thu, Jun 19, 2014 at 11​:57 AM, John Henning <john.l.henning@​gmail.com>
wrote​:

Package variable storage is allocated on demand. Therefore,
if you say​:

1 use strict;
2 use warnings;
3 our $flag;
4 ...
5 $flag = something

then the effect is​:
- storage for $flag is allocated at line 5, when $flag is
auto-instantiated
- line 3 gives the permission that allows this to happen
- line 2 will cause warnings if you try to create new
instances of something named $flag

You can't create an alias to a variable that doesn't exist. Storage is
allocated at line 3.

use strict;
use warnings;
BEGIN { print "1​: ", exists($​::{flag})?1​:0,"\n"; } print "4​: ",
exists($​::{flag})?1​:0,"\n";
our $flag;
BEGIN { print "2​: ", exists($​::{flag})?1​:0,"\n"; } print "5​: ",
exists($​::{flag})?1​:0,"\n";
$flag = 123;
BEGIN { print "3​: ", exists($​::{flag})?1​:0,"\n"; } print "6​: ",
exists($​::{flag})?1​:0,"\n";

1​: 0
2​: 1
3​: 1
4​: 1
5​: 1
6​: 1

I agree with David Golden. This should not be in the docs.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2014

From @xdg

Based on this conversation, I've updated the documentation in commit 0195d76

It removes mention of storage and offers some concrete examples.

It also moves the "use vars" comment to the end of the section to help discourage people from considering it as a real alternative.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2014

@xdg - Status changed from 'open' to 'pending release'

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 2, 2015

From @khwilliamson

Thanks for submitting this ticket

The issue should be resolved with the release today of Perl v5.22. If you find that the problem persists, feel free to reopen this ticket

--
Karl Williamson for the Perl 5 porters team

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 2, 2015

@khwilliamson - Status changed from 'pending release' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.