-
Notifications
You must be signed in to change notification settings - Fork 601
Add a work-around for older Perl's that need @{^CAPTURE}
#21678
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
Conversation
|
I'm not sure if this is the type of thing we want/need in the documentation, but it would have saved me about an hour of hair pulling if it were documented. |
@{^CAPTURE}@{^CAPTURE}
|
@mauke pointed out on IRC that this functionality is also in We could include this in the documentation if my function is too long/wordy. |
pod/perlvar.pod
Outdated
| function immediately after your regexp. | ||
|
|
||
| sub get_captures { | ||
| my $str = $&; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least in older Perls, the use of $& slows down all regex matches everywhere in the program.
pod/perlvar.pod
Outdated
| my @captures = (); | ||
|
|
||
| # Build array using the offsets from @- and @+ | ||
| for (my $i = 1; $i < @-; $i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use a C-style for loop here? This is a simple counting loop, so
for my $i (1 .. $#-) {expresses the idea more directly (and slightly more efficient).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree to disagree here... $#- is too much punctuation and not as easily understood.
I've been using Perl for 20+ years and I just learned about # syntax in arrays today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't like $#-, @- - 1 is the same thing. (Doesn't really look better with @-, though, IMHO).
pod/perlvar.pod
Outdated
| my $len = $end - $start; | ||
|
|
||
| my $str = substr($str, $start, $len); | ||
| push(@captures, $str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my @captures;
for (RANGE) {
push(@captures, ...)
}could also be written as
my @captures = map {
...
} RANGE;|
Upon further reflection I think it makes sense to use the format that @mauke proposed in |
|
I have quibbles about the commit messages, in particular the titles. A title should have enough information for someone scanning a log of commits to be able to quickly determine if this is potentially a commit that has bearing on what they're scanning for. For that, the title should include what the commit pertains to. For small specific changes like this, noting in the title that it is perlvar that is affected will tell me immediately if I should look further into it or not. I have adopted the convention for myself to place the file name at the beginning of the message, like perlvar: Use a faster/shorter version of get_captures() Something that affects more things wouldn't have the file name, but should indicate what area, as explicitly as possible, is affected. A commit message should answer the classic questions "Who, what, when, where, and why". The who and when are automatically filled in for you by the system, and appear adjacent to the title, but they are less important than the other three. A title should give at least a hint towards those. So "perlvar: Fix a typo" is better than "perlvar: Fix a word" because it more explicitly says what is wrong that needs to be fixed. |
|
@khwilliamson all that makes sense to me. Usually that all gets corrected on the squash and merge phase. Unless you'd like me to rebase and squash all these down to a single commit before then. |
|
On 12/6/23 09:11, Scott Baker wrote:
@khwilliamson <https://github.com/khwilliamson> all that makes sense to
me. Usually that all gets corrected on the squash and merge phase.
Unless you'd like me to rebase and squash all these down to a single
commit before then.
Just as long as it doesn't get merged with uncorrected wording is fine
with me
…
—
Reply to this email directly, view it on GitHub
<#21678 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA2DH7GJ6Y6VROINQM6I5DYICKMPAVCNFSM6AAAAABAAJXA5SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBTGIYTIOBWHE>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Ping... what's next on this? Is this something we need in the docs? |
|
I think now is a good time to squash and make sure the commit msg is good. Then I would look at the full result. |
b4ba397 to
61db135
Compare
|
@khwilliamson I have squashed |
|
@khwilliamson Thank you sir! |
I was bit by a bug wherein I was accessing
@{^CAPTURE}on Perl v5.16 which does not have this variable. This PR adds some documentation to allow a user a work-around if they are in the same situation.