Multiple-alias syntax for foreach

[nwc10]

Implement RFC 0001 - Multiple-alias syntax for foreach

I believe that the RFC process now is "this change is subject to normal code review and merging" by someone competent who is not me.

I guess we ask does it implement what the RFCs specified? in addition to the usual does the code have bugs or other defects? and on balance, does merging this give a net improvement?

[jkeenan]

On 6/23/21 4:57 AM, Nicholas Clark wrote: Implement RFC 0001 - /Multiple-alias syntax for foreach/ <https://github.com/Perl/RFCs/blob/master/rfc/rfc0001.md> [That link is correct but currently GitHub is reporting it as a 404. Start at https://github.com/Perl/RFCs/ <https://github.com/Perl/RFCs/> and you'll get links that are 404s. I'll edit the text to remove this note once GitHub no longer has its knickers in a twist]

No, the listed link is one character shorter than the link which you get by navigating through the above. Listed: https://github.com/Perl/RFCs/blob/master/rfc/rfc0001.md Confirmed: https://github.com/Perl/RFCs/blob/master/rfcs/rfc0001.md I'm not sure how you generated this email, so I don't know where in p.r. 18925 to go to fix. Can you investigate?

I believe that the RFC process now is "this change is subject to normal code review and merging" by *someone competent who is not me*.

This is curious. When I go to #18925 I see that I have been requested as a code reviewer. Should I have received an email making that request? AFAICT I did not receive one. Note that the p.r. has merge conflicts. Thank you very much. Jim Keenan

[Grinnz]

This is curious. When I go to #18925 I see that I have been requested as a code reviewer. Should I have received an email making that request? AFAICT I did not receive one.

You aren't currently assigned as a reviewer, those are just github's suggestions for who to assign as a reviewer (which it decides itself based on various factors AIUI).

[nwc10]

I don't know what triggered GitHub to send you an e-mail about this, because I don't think that it was anything I explicitly asked for. It has suggested you are a potential reviewer, but I didn't take it up on its suggestions.

Worse - I goofed, and fixed things, but it seems that it didn't e-mail you an updated version of the comment, so you're replying to things that I already figured out, fixed and edited away. Hateful software.

On 6/23/21 4:57 AM, Nicholas Clark wrote: Implement RFC 0001 - /Multiple-alias syntax for foreach/ https://github.com/Perl/RFCs/blob/master/rfc/rfc0001.md [That link is correct but currently GitHub is reporting it as a 404. Start at https://github.com/Perl/RFCs/ https://github.com/Perl/RFCs/ and you'll get links that are 404s. I'll edit the text to remove this note once GitHub no longer has its knickers in a twist]
No, the listed link is one character shorter than the link which you get by navigating through the above. Listed: https://github.com/Perl/RFCs/blob/master/rfc/rfc0001.md Confirmed: https://github.com/Perl/RFCs/blob/master/rfcs/rfc0001.md I'm not sure how you generated this email, so I don't know where in p.r. 18925 to go to fix. Can you investigate?

Yes, I figured that the problem was between keyboard and chair about 4 days ago, and fixed it in Perl/PPCs@6f2c56a

Specifically, I was getting confused between links that I had generated in the README.md that appeared at the bottom of the page, and links that it was generating at the top of the page, based on the files. However, I think I had hit some sort of confusion with force-pushes to my private test repository, where files didn't update immediately (could be wrong), so I was assuming I was seeing the same thing

I believe that the RFC process now is "this change is subject to normal code review and merging" by someone competent who is not me.
This is curious. When I go to #18925 I see that I have been requested as a code reviewer. Should I have received an email making that request? AFAICT I did not receive one. Note that the p.r. has merge conflicts. Thank you very much. Jim Keenan

(as above) - I didn't know who should be a code reviewer, but wasn't confident that its suggestions were great. Really, it's the grammar changes that I'm least confident about, and would like someone to say "this doesn't obviously add subtle bugs".

The conflict is due to other more recent changes to B::Concise in the lines around the $VERSION declaration. I've resolved the conflict, and pushed the whole branch rebased on blead. All the commits are identical.

[wolfsage]

The C level changes are a bit beyond me - would be nice to have someone like Dave or Tony review them (if they haven't already).

There are a number of whitespace oddities introduced that make the code hard to comprehend that would be nice if they were fixed up.

Have we done benchmarking to see how this impacts normal for loops?

Also worth doing if you haven't:

• -DDEBUGGING build
• Threaded build
• Look for any new warnings generated by gcc (and maybe clang)
• Maybe a new test.valgrind run but that can be painful

Overall I like the feature though I worry about the 'extra undefs if the list isn't quite the right size'.

I ... don't have a solution I like though so I guess I'm just complaining here.

Good stuff!

[rjbs]

@nwc10 The last I heard, this was waiting on code review. What's the current status of this branch? What prevents us from merging it?

Implementation problems? Lack of tests? Lack of docs? Lack of say-so?

[leonerd]

Oh if it's ready-pending-review I should definitely give a look-over.

[tonycoz]

This commit doesn't do what it says it does. It adds the block around the switch (which isn't purely a whitespace change), and doesn't actually indent the block.

Later commits add lines indented further, so the HEAD includes:

        STRLEN maxlen = 0;
        const char *max = SvPV_const(end, maxlen);
            bool pad_it = FALSE;
        if (DO_UTF8(end) && IN_UNI_8_BIT)

I suspect a git rebase --ignore-whitespace bit you here.

[nwc10]

It did. Fixed in the rebase, and thanks for correctly diagnosing the problem.

[nwc10]

@wolfsage - GitHub still thinks "changes requested" but I can't work out what it's nagging about. I think I've addressed all the specific comments you made (and everyone else's), although I only marked as "resolved" those that I was confident you would agree with.

[nwc10]

Pushing a rebase to avoid the conflicts.

[wolfsage]

One or two minor doc fixups but seems good to me (I only skimmed the C changes this time)

[nwc10]

I think that the "Sanity" test has failed because I pushed a commit onto the branch as-is, and as it's based on blead from before the recent release, "changed modules with unchanged VERSION" vs "the latest tag" differs (and so fails)

So a rebase is coming soon, to see if I can get to "All tests successful". It's not easy being green.

[smpeters]

I'm not sure how pedantic we want to be here, but the indentation looks off in Github because Deparse.pm is indented with tabs while the code added here is indented with spaces.

[nwc10]

I'm not sure either.

Usually I try to match the indenting (etc) style of the surrounding code, and I missed that this file had tabs.

Well, ...:

$ git show 8b7aa8e4305788f70f89b55541ae664e8ecb17e0 lib/B/Deparse.pm | perl -pe 's/\t/......../g'
commit 8b7aa8e4305788f70f89b55541ae664e8ecb17e0
Author: Nicholas Clark <nick@ccl4.org>
Date:   Mon Apr 26 11:23:33 2021 +0000

    B::Deparse now handles n-at-a-time for.

diff --git a/lib/B/Deparse.pm b/lib/B/Deparse.pm
index 2d33039a80..12f6a63670 100644
--- a/lib/B/Deparse.pm
+++ b/lib/B/Deparse.pm
@@ -52,7 +52,7 @@ use B qw(class main_root main_start main_cv svref_2object opnumber perlstring
         MDEREF_SHIFT
     );

-$VERSION = '1.58';
+$VERSION = '1.59';
 use strict;
 our $AUTOLOAD;
 use warnings ();
@@ -3954,7 +3954,18 @@ sub loop_common {
 ........} else {
 ........    $ary = $self->deparse($ary, 1);
 ........}
-........if (null $var) {
+        my $iter_targ = $kid->first->first->targ;
+        if ($iter_targ) {
+            # for my ($foo, $bar) () stores the count (less 1) in the targ of
+            # the ITER op.
+            my @vars;
+            my $targ = $enter->targ;
+            while ($iter_targ-- >= 0) {
+                push @vars, $self->padname_sv($targ)->PVX;
+                ++$targ;
+            }
+            $var = 'my (' . join(', ', @vars) . ')';
+        } elsif (null $var) {
             $var = $self->pp_padsv($enter, 1, 1);
 ........} elsif ($var->name eq "rv2gv") {
 ........    $var = $self->pp_rv2sv($var, 1);

it does and it doesn't. The line above my new block had a tab, but the first line after it did not.

The whole file is a inconsistent. There are parts of it with tabs, and other parts (I guess newer, but I didn't see a good way to check this) that have spaces.

So having discovered that, there doesn't seem to be any "right" answer, so I'm tempted to say that this part can be spaces.

[tonycoz]

I thought we converted lines we touched to spaces, from perlhack:

The codebase is a mixture of tabs and spaces for indentation, and we
are moving to spaces only.  Converting lines you're patching from 8-wide
tabs to spaces will help this migration.