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.

Sign up for GitHub

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

optimize head/tail #70

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

optimize head/tail #70

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97d5000
head/tail: avoid copying return values
dur-randir Feb 24, 2018
9cb01b1
head/tail: simplify boundary detection code
dur-randir Feb 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 14 additions & 32 deletions ListUtil.xs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,44 +623,26 @@ PPCODE:
{
int size = 0;
int start = 0;
int end = 0;
int i = 0;

size = SvIV( ST(0) );
if (items > 1) {
size = SvIV(*(SP + 1));

if ( ix == 0 ) {
start = 1;
end = start + size;
if ( size < 0 ) {
end += items - 1;
}
if ( end > items ) {
end = items;
}
}
else {
end = items;
if ( size < 0 ) {
start = -size + 1;
}
else {
start = end - size;
}
if ( start < 1 ) {
start = 1;
if (ix) start = items - size - 1;
start = 1 + (start + items - 1) % (items - 1);

if (size > items - 1) {
size = items - 1;
} else if (size < 0) {
size = size + items - 1;
}
}

if ( end < start ) {
XSRETURN(0);
}
else {
EXTEND( SP, end - start );
for ( i = start; i <= end; i++ ) {
PUSHs( sv_2mortal( newSVsv( ST(i) ) ) );
}
XSRETURN( end - start );
while (--size >= 0) {
++SP;
*SP = *(SP + start);
Copy link
Contributor

@leonerd leonerd Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. This is quite the change of behaviour - we're now returning aliases of the original input list, rather than new temporary SVs that copy it. This would have a change of behaviour in lvalue context; for example

my @n = (1 .. 10);
$_++ for tail 4, @n;

I'm undecided if that's a good thing or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If such code wouldn't work before, I'd say this would just be a new feature...

Copy link
Contributor

@leonerd leonerd Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be considered a feature yes. But it's a somewhat surprising change of behaviour. Is there a chance of accidental collateral damage?

I've asked for further opinion on p5p@ since it's a core module.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning aliases is the behavior I would expect.

}

PUTBACK;
}

void
Expand Down