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

av_undef's POD is confusing #12880

Closed
p5pRT opened this issue Mar 25, 2013 · 11 comments
Closed

av_undef's POD is confusing #12880

p5pRT opened this issue Mar 25, 2013 · 11 comments

Comments

@p5pRT
Copy link

p5pRT commented Mar 25, 2013

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

Searchable as RT117341$

@p5pRT
Copy link
Author

p5pRT commented Mar 25, 2013

From @bulk88

Created by @bulk88

av_undef's POD last got worked on in
http​://perl5.git.perl.org/perl.git/commit/8b9a1153f14d44cea2fcef118be9de0eea3dcaca?f=av.c
as part of Perl #107440 and
http​://perl5.git.perl.org/perl.git/commit/60edcf09a5cb026822f99270a4bfbe3149cfbb52?f=av.c
. Its current POD is
____________________________________________________________________
/*
=for apidoc av_undef

Undefines the array. Frees the memory used by the av to store its list of
scalars. If any destructors are triggered as a result, the av itself may
be freed.

=cut
*/
____________________________________________________________________

av_clear's POD says "Perl equivalent​: C<@​myarray = ();>." Wouldn't
av_undef be the same thing except av_undef frees more memory than an
av_clear would? So, what is the script level difference between av_clear
and av_undef? none?

Another problem is, " Frees the memory used by the av to store its list
of scalars. If any destructors are triggered as a result, the av itself
may be freed." implies that av_undef can be used instead of calling
SvREFCNT_dec/sv_2mortal. Someone else did get confused by that sentence
and did think av_undef frees an AV (SV) head. I can't think of a better
wording so I offer no patch. I didn't fully understand #107440.

Maybe " If any destructors are triggered as a result, the av itself may
be freed." can be replaced with "If the only refcount owner of the AV *,
is at the end of a chain of (circular) references that wind up owning
the only AV * refcount notch, the AV * will be freeded."

Perl Info

Flags:
    category=docs
    severity=medium

Site configuration information for perl 5.17.7:

Configured by Owner at Sun Dec 16 13:25:34 2012.

Summary of my perl5 (revision 5 version 17 subversion 7 patch blead 
2012-12-06.16:42:20 93a641ae382638ffd1980378be4810244d04f4b0 
v5.17.6-186-g93a641a) configuration:
  Snapshot of: 93a641ae382638ffd1980378be4810244d04f4b0
  Platform:
    osname=MSWin32, osvers=5.1, archname=MSWin32-x86-multi-thread
    uname=''
    config_args='undef'
    hint=recommended, useposix=true, d_sigaction=undef
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=undef, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cl', ccflags ='-nologo -GF -W3 -MD -Zi -DNDEBUG -O1 -GL -G7 
-DWIN32 -D_CONSOLE -DNO_STRICT  -DPERL_TEXTMODE_SCRIPTS 
-DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO 
-D_USE_32BIT_TIME_T',
    optimize='-MD -Zi -DNDEBUG -O1 -GL -G7',
    cppflags='-DWIN32'
    ccversion='13.10.6030', gccversion='', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64', 
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='link', ldflags ='-nologo -nodefaultlib -debug -opt:ref,icf 
-ltcg  -libpath:"c:\perl517\lib\CORE"  -machine:x86'
    libpth="C:\Program Files\Microsoft Visual Studio .NET 2003\VC7\lib"
    libs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib  
comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib  
netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib  version.lib 
odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
    perllibs=oldnames.lib kernel32.lib user32.lib gdi32.lib 
winspool.lib  comdlg32.lib advapi32.lib shell32.lib ole32.lib 
oleaut32.lib  netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib  
version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
    libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl517.lib
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug 
-opt:ref,icf -ltcg  -libpath:"c:\perl517\lib\CORE"  -machine:x86'

Locally applied patches:
    


@INC for perl 5.17.7:
    C:/perl517/site/lib
    C:/perl517/lib
    .


Environment for perl 5.17.7:
    HOME (unset)
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=C:\perl517\bin;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\IDE;C:\Program Files\Microsoft Visual Studio .NET 
2003\VC7\BIN;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\Tools;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\Tools\bin\prerelease;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\system32\wbem;
    PERL_BADLANG (unset)
    SHELL (unset)


@p5pRT
Copy link
Author

p5pRT commented Mar 26, 2013

From @iabyn

On Mon, Mar 25, 2013 at 08​:56​:15AM -0700, bulk88 wrote​:

. Its current POD is
____________________________________________________________________
/*
=for apidoc av_undef

Undefines the array. Frees the memory used by the av to store its list of
scalars. If any destructors are triggered as a result, the av itself may
be freed.

=cut
*/
____________________________________________________________________

av_clear's POD says "Perl equivalent​: C<@​myarray = ();>." Wouldn't
av_undef be the same thing except av_undef frees more memory than an
av_clear would? So, what is the script level difference between av_clear
and av_undef? none?

av_clear()​: @​array = ()A;
av_undef()​: undef @​array;

Another problem is, " Frees the memory used by the av to store its list
of scalars. If any destructors are triggered as a result, the av itself
may be freed." implies that av_undef can be used instead of calling
SvREFCNT_dec/sv_2mortal. Someone else did get confused by that sentence
and did think av_undef frees an AV (SV) head. I can't think of a better
wording so I offer no patch. I didn't fully understand #107440.

Maybe " If any destructors are triggered as a result, the av itself may
be freed." can be replaced with "If the only refcount owner of the AV *,
is at the end of a chain of (circular) references that wind up owning
the only AV * refcount notch, the AV * will be freeded."

That's not really right. It's not that there's a circular chain of
references; rather that a DESTROY() method could actively free the AV
itself, as the example in ticket showed​:

  sub A​::DESTROY{ $ra = 0 }
  $ra = [ bless [], 'A' ];
  undef @​$ra;

How about​:

  Warning​: it is possible that the actions of a destructor called
  directly or indirectly by freeing an element of the array, could cause
  the array itself to be freed; so you cannot rely on C<av> still
  pointing to a valid AV on return from the call.

--
It's not that I'm afraid to die, I just don't want to be there when it
happens.
  -- Woody Allen

@p5pRT
Copy link
Author

p5pRT commented Mar 26, 2013

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

@p5pRT
Copy link
Author

p5pRT commented Mar 26, 2013

From @ikegami

On Mon, Mar 25, 2013 at 8​:04 PM, Dave Mitchell <davem@​iabyn.com> wrote​:

That's not really right. It's not that there's a circular chain of
references; rather that a DESTROY() method could actively free the AV
itself, as the example in ticket showed​:

sub A&#8203;::DESTROY\{ $ra = 0 \}
$ra = \[ bless \[\]\, 'A' \];
undef @&#8203;$ra;

How about​:

Warning&#8203;: it is possible that the actions of a destructor called
directly or indirectly by freeing an element of the array\, could cause
the array itself to be freed; so you cannot rely on C\<av> still
pointing to a valid AV on return from the call\.

"...unless you increment its reference count first"?

@p5pRT
Copy link
Author

p5pRT commented Mar 1, 2014

From @bulk88

On Mon Mar 25 17​:04​:59 2013, davem wrote​:

On Mon, Mar 25, 2013 at 08​:56​:15AM -0700, bulk88 wrote​:

. Its current POD is
____________________________________________________________________
/*
=for apidoc av_undef

Undefines the array. Frees the memory used by the av to store its list of
scalars. If any destructors are triggered as a result, the av itself may
be freed.

=cut
*/
____________________________________________________________________

av_clear's POD says "Perl equivalent​: C<@​myarray = ();>." Wouldn't
av_undef be the same thing except av_undef frees more memory than an
av_clear would? So, what is the script level difference between av_clear
and av_undef? none?

av_clear()​: @​array = ()A;
av_undef()​: undef @​array;

Another problem is, " Frees the memory used by the av to store its list
of scalars. If any destructors are triggered as a result, the av itself
may be freed." implies that av_undef can be used instead of calling
SvREFCNT_dec/sv_2mortal. Someone else did get confused by that sentence
and did think av_undef frees an AV (SV) head. I can't think of a better
wording so I offer no patch. I didn't fully understand #107440.

Maybe " If any destructors are triggered as a result, the av itself may
be freed." can be replaced with "If the only refcount owner of the AV *,
is at the end of a chain of (circular) references that wind up owning
the only AV * refcount notch, the AV * will be freeded."

That's not really right. It's not that there's a circular chain of
references; rather that a DESTROY() method could actively free the AV
itself, as the example in ticket showed​:

sub A&#8203;::DESTROY\{ $ra = 0 \}
$ra = \[ bless \[\]\, 'A' \];
undef @&#8203;$ra;

How about​:

Warning&#8203;: it is possible that the actions of a destructor called
directly or indirectly by freeing an element of the array\, could cause
the array itself to be freed; so you cannot rely on C\<av> still
pointing to a valid AV on return from the call\.

Looking at this ticket again. I think the warning shouldn't be there at all about av_undef and destructors and self referential destruction. Wouldn't that then apply to av_store, sv_clear and sv_setsv(sv, &PL_sv_undef) too? and every SV manipulation function that can change a reference or manipulate an aggregate (HV/AV/GV)?

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Mar 19, 2014

From @bulk88

On Fri Feb 28 22​:36​:53 2014, bulk88 wrote​:

On Mon Mar 25 17​:04​:59 2013, davem wrote​:

On Mon, Mar 25, 2013 at 08​:56​:15AM -0700, bulk88 wrote​:

. Its current POD is
____________________________________________________________________
/*
=for apidoc av_undef

Undefines the array. Frees the memory used by the av to store its
list of
scalars. If any destructors are triggered as a result, the av
itself may
be freed.

=cut
*/
____________________________________________________________________

av_clear's POD says "Perl equivalent​: C<@​myarray = ();>." Wouldn't
av_undef be the same thing except av_undef frees more memory than
an
av_clear would? So, what is the script level difference between
av_clear
and av_undef? none?

av_clear()​: @​array = ()A;
av_undef()​: undef @​array;

Another problem is, " Frees the memory used by the av to store its
list
of scalars. If any destructors are triggered as a result, the av
itself
may be freed." implies that av_undef can be used instead of calling
SvREFCNT_dec/sv_2mortal. Someone else did get confused by that
sentence
and did think av_undef frees an AV (SV) head. I can't think of a
better
wording so I offer no patch. I didn't fully understand #107440.

Maybe " If any destructors are triggered as a result, the av itself
may
be freed." can be replaced with "If the only refcount owner of the
AV *,
is at the end of a chain of (circular) references that wind up
owning
the only AV * refcount notch, the AV * will be freeded."

That's not really right. It's not that there's a circular chain of
references; rather that a DESTROY() method could actively free the AV
itself, as the example in ticket showed​:

sub A​::DESTROY{ $ra = 0 }
$ra = [ bless [], 'A' ];
undef @​$ra;

How about​:

Warning​: it is possible that the actions of a destructor called
directly or indirectly by freeing an element of the array, could
cause
the array itself to be freed; so you cannot rely on C<av> still
pointing to a valid AV on return from the call.

Looking at this ticket again. I think the warning shouldn't be there
at all about av_undef and destructors and self referential
destruction. Wouldn't that then apply to av_store, sv_clear and
sv_setsv(sv, &PL_sv_undef) too? and every SV manipulation function
that can change a reference or manipulate an aggregate (HV/AV/GV)?

Bump.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Oct 6, 2015

From @jkeenan

On Mon Mar 25 08​:56​:15 2013, bulk88 wrote​:

This is a bug report for perl from bulk88@​hotmail.com,
generated with the help of perlbug 1.39 running under perl 5.17.7.

-----------------------------------------------------------------
[Please describe your issue here]

av_undef's POD last got worked on in
http​://perl5.git.perl.org/perl.git/commit/8b9a1153f14d44cea2fcef118be9de0eea3dcaca?f=av.c
as part of Perl #107440 and
http​://perl5.git.perl.org/perl.git/commit/60edcf09a5cb026822f99270a4bfbe3149cfbb52?f=av.c
. Its current POD is
____________________________________________________________________
/*
=for apidoc av_undef

Undefines the array. Frees the memory used by the av to store its
list of
scalars. If any destructors are triggered as a result, the av itself
may
be freed.

=cut
*/
____________________________________________________________________

av_clear's POD says "Perl equivalent​: C<@​myarray = ();>." Wouldn't
av_undef be the same thing except av_undef frees more memory than an
av_clear would? So, what is the script level difference between
av_clear
and av_undef? none?

Another problem is, " Frees the memory used by the av to store its
list
of scalars. If any destructors are triggered as a result, the av
itself
may be freed." implies that av_undef can be used instead of calling
SvREFCNT_dec/sv_2mortal. Someone else did get confused by that
sentence
and did think av_undef frees an AV (SV) head. I can't think of a
better
wording so I offer no patch. I didn't fully understand #107440.

Maybe " If any destructors are triggered as a result, the av itself
may
be freed." can be replaced with "If the only refcount owner of the AV
*,
is at the end of a chain of (circular) references that wind up owning
the only AV * refcount notch, the AV * will be freeded."

Could someone familiar with the Perl API comment on this documentation-related ticket dating from March 2013?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Oct 6, 2015

From @iabyn

On Fri, Feb 28, 2014 at 10​:36​:53PM -0800, bulk88 via RT wrote​:

Looking at this ticket again. I think the warning shouldn't be there at
all about av_undef and destructors and self referential destruction.
Wouldn't that then apply to av_store, sv_clear and sv_setsv(sv,
&PL_sv_undef) too? and every SV manipulation function that can change a
reference or manipulate an aggregate (HV/AV/GV)?

I'd argue that having the warnings for at least some functions is better
than no warnings at all.

How about the following diff, which attempts to rationalise the four
[ah]v_(clear|undef) pod entries​:

Inline Patch
diff --git a/av.c b/av.c
index 2c4740b..9a5644a 100644
--- a/av.c
+++ b/av.c
@@ -436,11 +436,15 @@ Perl_av_make(pTHX_ SSize_t size, SV **strp)
 /*
 =for apidoc av_clear
 
-Clears an array, making it empty.  Does not free the memory C<av> uses to
-store its list of scalars.  If any destructors are triggered as a result,
-C<av> itself may be freed when this function returns.
+Frees the all the elements of an array, leaving it empty.
+The XS equivalent of C<@array = ()>.  See also L</av_undef>.
 
-Perl equivalent: C<@myarray = ();>.
+Note that it is possible that the actions of a destructor called directly
+or indirectly by freeing an element of the array could cause the reference
+count of the array itself to be reduced (e.g. by deleting an entry in the
+symbol table). So it is a possibility that the AV could have been freed
+(or even reallocated) on return from the call unless you hold a reference
+to it.
 
 =cut
 */
@@ -500,9 +504,13 @@ Perl_av_clear(pTHX_ AV *av)
 /*
 =for apidoc av_undef
 
-Undefines the array.  Frees the memory used by the av to store its list of
-scalars.  If any destructors are triggered as a result, C<av> itself may
-be freed.
+Undefines the array. The XS equivalent of C<undef(@array)>.
+
+As well as freeing all the elements of the array (like C<av_clear()>), this
+also frees the memory used by the av to store its list of scalars.
+
+See L</av_clear> for a note about the array possibly being invalid on
+return.
 
 =cut
 */
diff --git a/hv.c b/hv.c
index 253cad9..3bab3e2 100644
--- a/hv.c
+++ b/hv.c
@@ -1599,8 +1599,8 @@ Perl_hv_delayfree_ent(pTHX_ HV *hv, HE *entry)
 Frees the all the elements of a hash, leaving it empty.
 The XS equivalent of C<%hash = ()>.  See also L</hv_undef>.
 
-If any destructors are triggered as a result, the hv itself may
-be freed.
+See L</av_clear> for a note about the hash possibly being invalid on
+return.
 
 =cut
 */
@@ -1834,10 +1834,8 @@ Undefines the hash.  The XS equivalent of C<undef(%hash)>.
 As well as freeing all the elements of the hash (like C<hv_clear()>), this
 also frees any auxiliary data and storage associated with the hash.
 
-If any destructors are triggered as a result, the hv itself may
-be freed.
-
-See also L</hv_clear>.
+See L</av_clear> for a note about the hash possibly being invalid on
+return.
 
 =cut
 */




-- 

"Emacs isn't a bad OS once you get used to it.
It just lacks a decent editor."

@p5pRT
Copy link
Author

p5pRT commented Oct 8, 2015

From @rurban

On Oct 6, 2015, at 4​:28 AM, James E Keenan via RT <perlbug-followup@​perl.org> wrote​:

On Mon Mar 25 08​:56​:15 2013, bulk88 wrote​:

This is a bug report for perl from bulk88@​hotmail.com,
generated with the help of perlbug 1.39 running under perl 5.17.7.

-----------------------------------------------------------------
[Please describe your issue here]

av_undef's POD last got worked on in
http​://perl5.git.perl.org/perl.git/commit/8b9a1153f14d44cea2fcef118be9de0eea3dcaca?f=av.c
as part of Perl #107440 and
http​://perl5.git.perl.org/perl.git/commit/60edcf09a5cb026822f99270a4bfbe3149cfbb52?f=av.c
. Its current POD is
____________________________________________________________________
/*
=for apidoc av_undef

Undefines the array. Frees the memory used by the av to store its
list of
scalars. If any destructors are triggered as a result, the av itself
may
be freed.

=cut
*/
____________________________________________________________________

av_clear's POD says "Perl equivalent​: C<@​myarray = ();>." Wouldn't
av_undef be the same thing except av_undef frees more memory than an
av_clear would?

av_clear is equivalent to @​a = ();
av_undef is equivalent to @​a = undef;

So, what is the script level difference between
av_clear and av_undef? none?

So on the script level they are very similar. av_undef changes the type.

Another problem is, " Frees the memory used by the av to store its
list of scalars. If any destructors are triggered as a result, the av
itself may be freed." implies that av_undef can be used instead of calling
SvREFCNT_dec/sv_2mortal.

No.
av_undef just clears the body, not the head. so you still need to free the head.

Someone else did get confused by that
sentence and did think av_undef frees an AV (SV) head. I can't think of a
better wording so I offer no patch. I didn't fully understand #107440.

Maybe " If any destructors are triggered as a result, the av itself
may be freed." can be replaced with "If the only refcount owner of the AV
*,
is at the end of a chain of (circular) references that wind up owning
the only AV * refcount notch, the AV * will be freeded.”

No, the current doc is better.
SvREFCNT_dec is still needed on an av_undef’ed array.

To the differences​:
AV's have an unshift optimization, where the head of the array (AvARRAY) can move
forth and back, but the buffer (AvALLOC) stays.
av_clear() / @​a = () will not change the AvALLOC part, only the AvARRAY part, the head.
This is properly documented.
av_clear will error with READONLY arrays.
With @​ISA the clearing is delayed

av_undef will clear the whole AvALLOC buffer.
With tied arrays, FILL with -1 is called.
@​ISA is destroyed immediately.

@p5pRT
Copy link
Author

p5pRT commented Oct 20, 2015

From @iabyn

On Tue, Oct 06, 2015 at 11​:52​:22AM +0100, Dave Mitchell wrote​:

How about the following diff, which attempts to rationalise the four
[ah]v_(clear|undef) pod entries​:

I've now pushed that as a4395eb.

--
The optimist believes that he lives in the best of all possible worlds.
As does the pessimist.

@p5pRT p5pRT closed this as completed Oct 28, 2015
@p5pRT
Copy link
Author

p5pRT commented Oct 28, 2015

@iabyn - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant