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

%{^CAPTURE_ALL} is %+, not %-. #16105

Closed
p5pRT opened this issue Aug 9, 2017 · 31 comments
Closed

%{^CAPTURE_ALL} is %+, not %-. #16105

p5pRT opened this issue Aug 9, 2017 · 31 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Aug 9, 2017

Migrated from rt.perl.org#131867 (status was 'pending release')

Searchable as RT131867$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 9, 2017

From @Abigail

Created by @Abigail

perlvar states that %{^CAPTURE_ALL} is an alias to %-.
But it isn't, it's an alias to %+ (just like %{^CAPTURE} is).

  #!/opt/perl/bin/perl

  use 5.026;

  use strict;
  use warnings;
  no warnings 'syntax';

  "AB" =~ /(?<letter>A)(?<letter>B)/;

  use Data​::Dumper;
  print Dumper (\%-);
  print Dumper (\%{^CAPTURE_ALL});
  print Dumper (\%+);

  __END__
  $VAR1 = {
  'letter' => [
  'A',
  'B'
  ]
  };
  $VAR1 = {
  'letter' => 'A'
  };
  $VAR1 = {
  'letter' => 'A'
  };

This issue is still present in blead.

Perl Info

Flags:
    category=core
    severity=medium

Site configuration information for perl 5.26.0:

Configured by abigail at Wed Jun  7 23:04:17 CEST 2017.

Summary of my perl5 (revision 5 version 26 subversion 0) configuration:
   
  Platform:
    osname=darwin
    osvers=15.6.0
    archname=darwin-ld-2level
    uname='darwin athena 15.6.0 darwin kernel version 15.6.0: thu jun 23 18:25:34 pdt 2016; root:xnu-3248.60.10~1release_x86_64 x86_64 '
    config_args='-des -Uversiononly -Dperladmin=abigail@abigail.be -Dcf_email=abigail@abigail.be -Dmydomain=abigail.be -Dcc=gcc -Dprefix=/opt/perl/5.26.0 -Dusedevel -Dusemorebits'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=define
    usemymalloc=n
    default_inc_excludes_dot=define
    bincompat5005=undef
  Compiler:
    cc='gcc'
    ccflags ='-fno-common -DPERL_DARWIN -no-cpp-precomp -mmacosx-version-min=10.11 -fno-strict-aliasing -pipe -fstack-protector-strong -I/opt/local/include -DPERL_USE_SAFE_PUTENV'
    optimize='-O3'
    cppflags='-no-cpp-precomp -fno-common -DPERL_DARWIN -no-cpp-precomp -mmacosx-version-min=10.11 -fno-strict-aliasing -pipe -fstack-protector-strong -I/opt/local/include'
    ccversion=''
    gccversion='4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.42.1)'
    gccosandvers=''
    intsize=4
    longsize=8
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long'
    ivsize=8
    nvtype='long double'
    nvsize=16
    Off_t='off_t'
    lseeksize=8
    alignbytes=16
    prototype=define
  Linker and Libraries:
    ld='gcc'
    ldflags =' -mmacosx-version-min=10.11 -fstack-protector-strong -L/opt/local/lib'
    libpth=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/8.0.0/lib /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/lib /opt/local/lib /usr/lib
    libs=-lpthread -lgdbm -ldbm -ldl -lm -lutil -lc
    perllibs=-lpthread -ldl -lm -lutil -lc
    libc=
    so=dylib
    useshrplib=false
    libperl=libperl.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=bundle
    d_dlsymun=undef
    ccdlflags=' '
    cccdlflags=' '
    lddlflags=' -mmacosx-version-min=10.11 -bundle -undefined dynamic_lookup -L/opt/local/lib -fstack-protector-strong'



@INC for perl 5.26.0:
    /Users/abigail/Perl/CPAN/Regexp-Common2/lib
    /Users/abigail/Perl/CPAN/Test-Regexp/lib
    /opt/perl/5.26.0/lib/site_perl/5.26.0/darwin-ld-2level
    /opt/perl/5.26.0/lib/site_perl/5.26.0
    /opt/perl/5.26.0/lib/5.26.0/darwin-ld-2level
    /opt/perl/5.26.0/lib/5.26.0


Environment for perl 5.26.0:
    DYLD_LIBRARY_PATH (unset)
    HOME=/Users/abigail
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH=/Users/abigail/Lib:/usr/local/lib:/usr/lib:/lib:/usr/X11R6/lib
    LOGDIR (unset)
    PATH=/Users/abigail/Bin:/opt/perl/bin:/opt/local/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/usr/X11R6/bin:/usr/games:/opt/git/bin:/Users/abigail/Perl/Photos:/Users/abigail/Perl/Bin:/opt/mysql/bin:/opt/local/bin:/Users/abigail/bin
    PERL5LIB=/Users/abigail/Perl/CPAN/Regexp-Common2/lib:/Users/abigail/Perl/CPAN/Test-Regexp/lib
    PERLDIR=/opt/perl
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 9, 2017

From @jkeenan

On 08/09/2017 07​:32 AM, Abigail wrote​:

# New Ticket Created by Abigail
# Please include the string​: [perl #131867]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=131867 >

This is a bug report for perl from abigail@​abigail.be,
generated with the help of perlbug 1.40 running under perl 5.26.0.

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

perlvar states that %{^CAPTURE_ALL} is an alias to %-.
But it isn't, it's an alias to %+ (just like %{^CAPTURE} is).

 \#\!/opt/perl/bin/perl

 use 5\.026;

 use strict;
 use warnings;
 no    warnings 'syntax';


 "AB" =~ /\(?\<letter>A\)\(?\<letter>B\)/;

 use Data&#8203;::Dumper;
 print Dumper \(\\%\-\);
 print Dumper \(\\%\{^CAPTURE\_ALL\}\);
 print Dumper \(\\%\+\);

 \_\_END\_\_
 $VAR1 = \{
      'letter' => \[
            'A'\,
            'B'
          \]
    \};
 $VAR1 = \{
      'letter' => 'A'
    \};
 $VAR1 = \{
      'letter' => 'A'
    \};

This issue is still present in blead.

Is this just a documentation error or is the implementation itself wrong?

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 9, 2017

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 9, 2017

From @Abigail

On Wed, Aug 09, 2017 at 04​:41​:04AM -0700, James E Keenan via RT wrote​:

On 08/09/2017 07​:32 AM, Abigail wrote​:

Is this just a documentation error or is the implementation itself wrong?

The implemenation it wrong; %+ already has an alias, %{^CAPTURE}.
%{^CAPTURE_ALL} is intended to be an alias for %-, instead of
a second alias for %+, leaving %- without an alias.

Abigail

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 9, 2017

From @jkeenan

On 08/09/2017 09​:15 AM, Abigail wrote​:

On Wed, Aug 09, 2017 at 04​:41​:04AM -0700, James E Keenan via RT wrote​:

On 08/09/2017 07​:32 AM, Abigail wrote​:

Is this just a documentation error or is the implementation itself wrong?

The implemenation it wrong; %+ already has an alias, %{^CAPTURE}.
%{^CAPTURE_ALL} is intended to be an alias for %-, instead of
a second alias for %+, leaving %- without an alias.

Will we have to have a deprecation cycle in order to correct this problem?

(From what I understand, yes.)

Thank you very much.
jimk

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 10, 2017

From @demerphq

On 9 Aug 2017 15​:00, "James E Keenan" <jkeenan@​pobox.com> wrote​:

On 08/09/2017 09​:15 AM, Abigail wrote​:

On Wed, Aug 09, 2017 at 04​:41​:04AM -0700, James E Keenan via RT wrote​:

On 08/09/2017 07​:32 AM, Abigail wrote​:

Is this just a documentation error or is the implementation itself wrong?

The implemenation it wrong; %+ already has an alias, %{^CAPTURE}.
%{^CAPTURE_ALL} is intended to be an alias for %-, instead of
a second alias for %+, leaving %- without an alias.

Will we have to have a deprecation cycle in order to correct this problem?

(From what I understand, yes.)

No I don't think so. This was just a stupid oversight. The docs are correct
the implementation is broken.

My bad. Sorry about this.

Yves

Thank you very much.
jimk

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 12, 2017

From @Abigail

On Thu, Aug 10, 2017 at 05​:23​:50AM -0700, yves orton via RT wrote​:

On 9 Aug 2017 15​:00, "James E Keenan" <jkeenan@​pobox.com> wrote​:

On 08/09/2017 09​:15 AM, Abigail wrote​:

On Wed, Aug 09, 2017 at 04​:41​:04AM -0700, James E Keenan via RT wrote​:

On 08/09/2017 07​:32 AM, Abigail wrote​:

Is this just a documentation error or is the implementation itself wrong?

The implemenation it wrong; %+ already has an alias, %{^CAPTURE}.
%{^CAPTURE_ALL} is intended to be an alias for %-, instead of
a second alias for %+, leaving %- without an alias.

Will we have to have a deprecation cycle in order to correct this problem?

(From what I understand, yes.)

No I don't think so. This was just a stupid oversight. The docs are correct
the implementation is broken.

I agree with Yves. It's new in 5.26.0. I haven't looked how it's implemented
yet, but I wouldn't be surprised if it's a one character fix. If possible,
it should get fixed in 5.26.1.

My bad. Sorry about this.

Don't fret it. Stuff happens; I only found this issue when writing
my slides for YAPC​::EU.

Abigail

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 13, 2017

From @xsawyerx

On 08/12/2017 12​:19 PM, Abigail wrote​:

On Thu, Aug 10, 2017 at 05​:23​:50AM -0700, yves orton via RT wrote​:

On 9 Aug 2017 15​:00, "James E Keenan" <jkeenan@​pobox.com> wrote​:

On 08/09/2017 09​:15 AM, Abigail wrote​:

On Wed, Aug 09, 2017 at 04​:41​:04AM -0700, James E Keenan via RT wrote​:

On 08/09/2017 07​:32 AM, Abigail wrote​:

Is this just a documentation error or is the implementation itself wrong?

The implemenation it wrong; %+ already has an alias, %{^CAPTURE}.
%{^CAPTURE_ALL} is intended to be an alias for %-, instead of
a second alias for %+, leaving %- without an alias.

Will we have to have a deprecation cycle in order to correct this problem?

(From what I understand, yes.)

No I don't think so. This was just a stupid oversight. The docs are correct
the implementation is broken.

I agree with Yves. It's new in 5.26.0. I haven't looked how it's implemented
yet, but I wouldn't be surprised if it's a one character fix. If possible,
it should get fixed in 5.26.1.

I agree.

We already intend for it to provide a certain behavior based on
documentation and all announcements. This should be classified as a bug
and since it's new, we can't argue people heavily use it at this point.
Otherwise, they would have provided a ticket. as Abigail did.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 13, 2017

From @khwilliamson

On 08/13/2017 08​:24 AM, Sawyer X wrote​:

On 08/12/2017 12​:19 PM, Abigail wrote​:

On Thu, Aug 10, 2017 at 05​:23​:50AM -0700, yves orton via RT wrote​:

On 9 Aug 2017 15​:00, "James E Keenan" <jkeenan@​pobox.com> wrote​:

On 08/09/2017 09​:15 AM, Abigail wrote​:

On Wed, Aug 09, 2017 at 04​:41​:04AM -0700, James E Keenan via RT wrote​:

On 08/09/2017 07​:32 AM, Abigail wrote​:

Is this just a documentation error or is the implementation itself wrong?

The implemenation it wrong; %+ already has an alias, %{^CAPTURE}.
%{^CAPTURE_ALL} is intended to be an alias for %-, instead of
a second alias for %+, leaving %- without an alias.

Will we have to have a deprecation cycle in order to correct this problem?

(From what I understand, yes.)

No I don't think so. This was just a stupid oversight. The docs are correct
the implementation is broken.

I agree with Yves. It's new in 5.26.0. I haven't looked how it's implemented
yet, but I wouldn't be surprised if it's a one character fix. If possible,
it should get fixed in 5.26.1.

I agree.

We already intend for it to provide a certain behavior based on
documentation and all announcements. This should be classified as a bug
and since it's new, we can't argue people heavily use it at this point.
Otherwise, they would have provided a ticket. as Abigail did.

We have agreed in past releases that new features are eligible for
patches in a dot release; otherwise they aren't usable until the next
major one, which is absurd.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 22, 2017

From kent@setattr.net

This is a simple fix - I assume that _tie_it is used by core only and can rely on the name of GV. The patch also adds few tests for the named variables.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 22, 2017

From kent@setattr.net

blead-capture_all.patch
diff --git a/ext/Tie-Hash-NamedCapture/NamedCapture.xs b/ext/Tie-Hash-NamedCapture/NamedCapture.xs
index 7eaae5614d..a607c10090 100644
--- a/ext/Tie-Hash-NamedCapture/NamedCapture.xs
+++ b/ext/Tie-Hash-NamedCapture/NamedCapture.xs
@@ -25,8 +25,11 @@ _tie_it(SV *sv)
     GV * const gv = (GV *)sv;
     HV * const hv = GvHVn(gv);
     SV *rv = newSV_type(SVt_RV);
+    const char *gv_name = GvNAME(gv);
   CODE:
-    SvRV_set(rv, newSVuv(*GvNAME(gv) == '-' ? RXapif_ALL : RXapif_ONE));
+    SvRV_set(rv, newSVuv(
+        strEQ(gv_name, "-") || strEQ(gv_name, "\003APTURE_ALL")
+            ? RXapif_ALL : RXapif_ONE));
     SvROK_on(rv);
     sv_bless(rv, GvSTASH(CvGV(cv)));
 
diff --git a/ext/Tie-Hash-NamedCapture/t/tiehash.t b/ext/Tie-Hash-NamedCapture/t/tiehash.t
index 3ebc81ad68..962754085f 100644
--- a/ext/Tie-Hash-NamedCapture/t/tiehash.t
+++ b/ext/Tie-Hash-NamedCapture/t/tiehash.t
@@ -3,7 +3,12 @@ use strict;
 
 use Test::More;
 
-my %hashes = ('+' => \%+, '-' => \%-);
+my %hashes = (
+    '+' => \%+,
+    '-' => \%-,
+    '{^CAPTURE}' => \%{^CAPTURE},
+    '{^CAPTURE_ALL}' => \%{^CAPTURE_ALL},
+);
 
 foreach (['plus1'],
 	 ['minus1', all => 1],
@@ -20,12 +25,12 @@ foreach (['plus1'],
 is("abcdef" =~ /(?<foo>[ab])*(?<bar>c)(?<foo>d)(?<bar>[ef]*)/, 1,
    "We matched");
 
-foreach my $name (qw(+ plus1 plus2 plus3)) {
+foreach my $name (qw(+ {^CAPTURE} plus1 plus2 plus3)) {
     my $hash = $hashes{$name};
     is_deeply($hash, { foo => 'b', bar => 'c' }, "%$name is as expected");
 }
 
-foreach my $name (qw(- minus1 minus2)) {
+foreach my $name (qw(- {^CAPTURE_ALL} minus1 minus2)) {
     my $hash = $hashes{$name};
     is_deeply($hash, { foo => [qw(b d)], bar => [qw(c ef)] },
 	      "%$name is as expected");
@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 22, 2017

From @jkeenan

On Fri, 22 Sep 2017 16​:41​:03 GMT, kent-perl@​setattr.net wrote​:

This is a simple fix - I assume that _tie_it is used by core only and
can rely on the name of GV. The patch also adds few tests for the
named variables.

This patch is available for smoke testing in this branch​:

smoke-me/jkeenan/131867-capture-all

Thank you very much.
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 23, 2017

From @cpansprout

On Fri, 22 Sep 2017 09​:41​:03 -0700, kent-perl@​setattr.net wrote​:

This is a simple fix - I assume that _tie_it is used by core only and
can rely on the name of GV.

I can confirm that only the core uses (is supposed to use) _tie_it.

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From @haukex

Hi,

Bump! Also, I'm confused by the source in gv.c (below), as it *looks* like %{^CAPTURE} is being aliased to %-, and %{^CAPTURE_ALL} to %+, even though according to the documentation, it should be the other way around. But when inspected (attached), *both* %{^CAPTURE} and %{^CAPTURE_ALL} look like %+ ...

/* @​{^CAPTURE} %{^CAPTURE} */
if (memEQs(name, len, "\003APTURE")) {
  AV* const av = GvAVn(gv);
  const Size_t n = *name;
 
  sv_magic(MUTABLE_SV(av), (SV*)n, PERL_MAGIC_regdata, NULL, 0);
  SvREADONLY_on(av);
 
  if (sv_type == SVt_PVHV || sv_type == SVt_PVGV)
  require_tie_mod_s(gv, '-', "Tie​::Hash​::NamedCapture",0);
 
} else /* %{^CAPTURE_ALL} */
if (memEQs(name, len, "\003APTURE_ALL")) {
  if (sv_type == SVt_PVHV || sv_type == SVt_PVGV)
  require_tie_mod_s(gv, '+', "Tie​::Hash​::NamedCapture",0);
}

For more context​: https://www.perlmonks.org/?node_id=11101258

Regards,
-- Hauke D

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From [Unknown Contact. See original ticket]

Hi,

Bump! Also, I'm confused by the source in gv.c (below), as it *looks* like %{^CAPTURE} is being aliased to %-, and %{^CAPTURE_ALL} to %+, even though according to the documentation, it should be the other way around. But when inspected (attached), *both* %{^CAPTURE} and %{^CAPTURE_ALL} look like %+ ...

/* @​{^CAPTURE} %{^CAPTURE} */
if (memEQs(name, len, "\003APTURE")) {
  AV* const av = GvAVn(gv);
  const Size_t n = *name;
 
  sv_magic(MUTABLE_SV(av), (SV*)n, PERL_MAGIC_regdata, NULL, 0);
  SvREADONLY_on(av);
 
  if (sv_type == SVt_PVHV || sv_type == SVt_PVGV)
  require_tie_mod_s(gv, '-', "Tie​::Hash​::NamedCapture",0);
 
} else /* %{^CAPTURE_ALL} */
if (memEQs(name, len, "\003APTURE_ALL")) {
  if (sv_type == SVt_PVHV || sv_type == SVt_PVGV)
  require_tie_mod_s(gv, '+', "Tie​::Hash​::NamedCapture",0);
}

For more context​: https://www.perlmonks.org/?node_id=11101258

Regards,
-- Hauke D

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From @hvds

On Wed, 12 Jun 2019 02​:29​:12 -0700, haukex@​zero-g.net wrote​:

Bump! Also, I'm confused by the source in gv.c (below), as it *looks*
like %{^CAPTURE} is being aliased to %-, and %{^CAPTURE_ALL} to %+,
[...]
require_tie_mod_s(gv, '-', "Tie​::Hash​::NamedCapture",0);

Looks can be deceiving​: the '-' or '+' in the second argument is there only for diagnostics and is not passed through to the function that does the actual tying, which uses the name found in the gv.

The patch seems to have got dropped on the floor somehow - Jim, you mention a smoke branch, did that raise any concerns?

Hugo

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From @jkeenan

On Wed, 12 Jun 2019 10​:27​:12 GMT, hv wrote​:

On Wed, 12 Jun 2019 02​:29​:12 -0700, haukex@​zero-g.net wrote​:

Bump! Also, I'm confused by the source in gv.c (below), as it *looks*
like %{^CAPTURE} is being aliased to %-, and %{^CAPTURE_ALL} to %+,
[...]
require_tie_mod_s(gv, '-', "Tie​::Hash​::NamedCapture",0);

Looks can be deceiving​: the '-' or '+' in the second argument is there
only for diagnostics and is not passed through to the function that
does the actual tying, which uses the name found in the gv.

The patch seems to have got dropped on the floor somehow - Jim, you
mention a smoke branch, did that raise any concerns?

Hugo

http​://perl.develop-help.com/?b=smoke-me%2Fjkeenan%2F131867-capture-all

But we have much fewer smoke-testing back when the patch was submitted. So if we're going to do anything we should rebase the branch on blead and re-smoke. Let me know what you recommend.

Thank you very much.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From @hvds

On Wed, 12 Jun 2019 05​:00​:45 -0700, jkeenan wrote​:

On Wed, 12 Jun 2019 10​:27​:12 GMT, hv wrote​:

The patch seems to have got dropped on the floor somehow - Jim, you
mention a smoke branch, did that raise any concerns?

http​://perl.develop-help.com/?b=smoke-me%2Fjkeenan%2F131867-capture-
all

But we have much fewer smoke-testing back when the patch was
submitted. So if we're going to do anything we should rebase the
branch on blead and re-smoke. Let me know what you recommend.

Well we should do something, so let's do that.

I'll also add it to various blockers tickets if I can find them, so we at least consider it for maintenance releases​: it'd be a shame to have people start to use this correctly, and then find it does the wrong thing on some releases.

Hugo

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From @jkeenan

On Wed, 12 Jun 2019 12​:20​:08 GMT, hv wrote​:

On Wed, 12 Jun 2019 05​:00​:45 -0700, jkeenan wrote​:

On Wed, 12 Jun 2019 10​:27​:12 GMT, hv wrote​:

The patch seems to have got dropped on the floor somehow - Jim, you
mention a smoke branch, did that raise any concerns?

http​://perl.develop-help.com/?b=smoke-me%2Fjkeenan%2F131867-capture-
all

But we have much fewer smoke-testing back when the patch was
submitted. So if we're going to do anything we should rebase the
branch on blead and re-smoke. Let me know what you recommend.

Well we should do something, so let's do that.

Rebased and pushed.

I'll also add it to various blockers tickets if I can find them, so we
at least consider it for maintenance releases​: it'd be a shame to have
people start to use this correctly, and then find it does the wrong
thing on some releases.

Hugo

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From @haukex

Hi,

I suspect this is related​: As PerlMonks user vr noted (https://www.perlmonks.org/?node_id=11101258), in the following, swapping the two "dd" lines causes %{^CAPTURE} to show up as an empty, untied hash. Can this be fixed in the same fix as this bug, or should it be a new bug report?

use Data​::Dump;
"a aa aaa aaaa" =~ /(?<a>a+) (?<a>a+) (?​:(?<a>a+)bbb)?/;
dd \%{^CAPTURE};
dd \@​{^CAPTURE};

Thanks,
-- Hauke D

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From [Unknown Contact. See original ticket]

Hi,

I suspect this is related​: As PerlMonks user vr noted (https://www.perlmonks.org/?node_id=11101258), in the following, swapping the two "dd" lines causes %{^CAPTURE} to show up as an empty, untied hash. Can this be fixed in the same fix as this bug, or should it be a new bug report?

use Data​::Dump;
"a aa aaa aaaa" =~ /(?<a>a+) (?<a>a+) (?​:(?<a>a+)bbb)?/;
dd \%{^CAPTURE};
dd \@​{^CAPTURE};

Thanks,
-- Hauke D

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From @haukex

Hi,

On Wed, 12 Jun 2019 03​:27​:12 -0700, hv wrote​:

On Wed, 12 Jun 2019 02​:29​:12 -0700, haukex@​zero-g.net wrote​:

Bump! Also, I'm confused by the source in gv.c (below), as it *looks*
like %{^CAPTURE} is being aliased to %-, and %{^CAPTURE_ALL} to %+,
[...]
require_tie_mod_s(gv, '-', "Tie​::Hash​::NamedCapture",0);

Looks can be deceiving​: the '-' or '+' in the second argument is there
only for diagnostics and is not passed through to the function that
does the actual tying, which uses the name found in the gv.

Ah, thank you! So does that mean they'll be reported wrong in diagnostics? And I guess it would probably make sense to change them in any case? (I don't see the patch in this thread doing that.)

Thanks, Regards,
-- Hauke D

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From @hvds

On Wed, 12 Jun 2019 10​:43​:40 -0700, haukex@​zero-g.net wrote​:

Hi,

On Wed, 12 Jun 2019 03​:27​:12 -0700, hv wrote​:

On Wed, 12 Jun 2019 02​:29​:12 -0700, haukex@​zero-g.net wrote​:

Bump! Also, I'm confused by the source in gv.c (below), as it
*looks*
like %{^CAPTURE} is being aliased to %-, and %{^CAPTURE_ALL} to %+,
[...]
require_tie_mod_s(gv, '-', "Tie​::Hash​::NamedCapture",0);

Looks can be deceiving​: the '-' or '+' in the second argument is
there
only for diagnostics and is not passed through to the function that
does the actual tying, which uses the name found in the gv.

Ah, thank you! So does that mean they'll be reported wrong in
diagnostics? And I guess it would probably make sense to change them
in any case? (I don't see the patch in this thread doing that.)

That means if the internal require_tie_mod_s() call fails, it will refer to %- in the message. The call it is making accepts only a character here, not a string, so making it accept '^CAPTURE' would require a more extensive change (and it isn't obvious to me what other impact such a change might have).

The diagnostics you might get would normally be possible only if your perl installation is broken, eg if the Tie​::Hash​::NamedCapture module cannot be loaded.

Hugo

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From @hvds

On Wed, 12 Jun 2019 10​:35​:13 -0700, haukex@​zero-g.net wrote​:

Hi,

I suspect this is related​: As PerlMonks user vr noted
(https://www.perlmonks.org/?node_id=11101258), in the following,
swapping the two "dd" lines causes %{^CAPTURE} to show up as an empty,
untied hash. Can this be fixed in the same fix as this bug, or should
it be a new bug report?

use Data​::Dump;
"a aa aaa aaaa" =~ /(?<a>a+) (?<a>a+) (?​:(?<a>a+)bbb)?/;
dd \%{^CAPTURE};
dd \@​{^CAPTURE};

That appears to be a separate bug, I suggest filing a new bug report that shows the output you get in both cases (preferably using a core module such as Data​::Dumper).

At first glance, it looks like the implementation in gv_magicalize() is quite flawed​: the docs say this function is called "when creating a new GV", so having the tie action be conditional on the svtype seems wrong. However there's similar logic in the handling of %+ (which doesn't suffer from the same problem), so I may be misunderstanding what's going on.

Hugo

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From @haukex

Hi,

On Wed, 12 Jun 2019 12​:03​:16 -0700, hv wrote​:

The call it is making accepts only a
character here, not a string, so making it accept '^CAPTURE' would
require a more extensive change (and it isn't obvious to me what other
impact such a change might have).

Ok, but shouldn't at least the '+' and '-' be swapped?

Thanks,
-- Hauke D

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From [Unknown Contact. See original ticket]

Hi,

On Wed, 12 Jun 2019 12​:03​:16 -0700, hv wrote​:

The call it is making accepts only a
character here, not a string, so making it accept '^CAPTURE' would
require a more extensive change (and it isn't obvious to me what other
impact such a change might have).

Ok, but shouldn't at least the '+' and '-' be swapped?

Thanks,
-- Hauke D

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From @haukex

Hi,

On Wed, 12 Jun 2019 12​:32​:59 -0700, hv wrote​:

That appears to be a separate bug, I suggest filing a new bug report
that shows the output you get in both cases (preferably using a core
module such as Data​::Dumper).

Ok, thanks, done​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=134193

Regards,
-- Hauke D

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 12, 2019

From [Unknown Contact. See original ticket]

Hi,

On Wed, 12 Jun 2019 12​:32​:59 -0700, hv wrote​:

That appears to be a separate bug, I suggest filing a new bug report
that shows the output you get in both cases (preferably using a core
module such as Data​::Dumper).

Ok, thanks, done​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=134193

Regards,
-- Hauke D

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 19, 2019

From @tonycoz

On Fri, 22 Sep 2017 09​:41​:03 -0700, kent-perl@​setattr.net wrote​:

This is a simple fix - I assume that _tie_it is used by core only and
can rely on the name of GV. The patch also adds few tests for the
named variables.

Thanks, applied as 1a1d29a along with the patches for 134193.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 19, 2019

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

@p5pRT p5pRT closed this Jun 19, 2019
@toddr toddr added this to the 5.32.0 milestone Oct 25, 2019
@p5pRT p5pRT added the 5.30.1 label Oct 25, 2019
@toddr toddr modified the milestones: 5.32.0, 5.30.1 Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.