#53405 bugfix: current in foreach (by ptrofimov,dmkuznetsov) #113

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@ptrofimov

#53405 bugfix: current in foreach (by ptrofimov,dmkuznetsov)

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Sep 18, 2012

Contributor

Could you please add a test for this bugfix?

Contributor

lstrojny commented Sep 18, 2012

Could you please add a test for this bugfix?

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Jan 6, 2013

Contributor

ping @ptrofimov

Contributor

lstrojny commented Jan 6, 2013

ping @ptrofimov

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Jan 14, 2013

Contributor

ping again @ptrofimov

Contributor

lstrojny commented Jan 14, 2013

ping again @ptrofimov

@ptrofimov

This comment has been minimized.

Show comment
Hide comment
@ptrofimov

ptrofimov Feb 11, 2013

@lstrojny Oh, sorry. I thought everybody forgot about this pull request)) I'll add tests in few days.

@lstrojny Oh, sorry. I thought everybody forgot about this pull request)) I'll add tests in few days.

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Feb 11, 2013

Contributor

Nope. We never forget (alright, sometimes).

Contributor

lstrojny commented Feb 11, 2013

Nope. We never forget (alright, sometimes).

@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic Feb 11, 2013

Member

I don't think that this change is right. I'm pretty sure that the function was intentionally marked ZEND_SEND_PREFER_REF, just like key() was also.

This will change the behavior and it's unclear to me what behavior is the right one. At least this needs a bit more explanation.

If this is about making current()/key() independent of foreach() then this will only help in a limited way (esp when foreach-by-ref or foreach-over-ref is used). I have a patch that properly decouples foreach from the internal array pointer, but that's a rather more heavy change.

Member

nikic commented Feb 11, 2013

I don't think that this change is right. I'm pretty sure that the function was intentionally marked ZEND_SEND_PREFER_REF, just like key() was also.

This will change the behavior and it's unclear to me what behavior is the right one. At least this needs a bit more explanation.

If this is about making current()/key() independent of foreach() then this will only help in a limited way (esp when foreach-by-ref or foreach-over-ref is used). I have a patch that properly decouples foreach from the internal array pointer, but that's a rather more heavy change.

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Feb 11, 2013

Contributor

@nikic thanks for the explanation. Do you mind closing it and open a different issue for your refactoring?

Contributor

lstrojny commented Feb 11, 2013

@nikic thanks for the explanation. Do you mind closing it and open a different issue for your refactoring?

@ptrofimov

This comment has been minimized.

Show comment
Hide comment
@ptrofimov

ptrofimov Feb 13, 2013

Well, the problem was that the code

$a = array(1, 2, 3);
foreach ($a as $i) {
    var_dump(current($a));
}

should give 1,1,1. But it gives 2,2,2. After this patch it gave 1,1,1.
But just the other day I downloaded latest source and wrote the test for this, and, unfortunately, it gives 2,3,false.
Why it is so, I don't know, may be something changed in new version.

Well, perhaps, we should close the issue((

Well, the problem was that the code

$a = array(1, 2, 3);
foreach ($a as $i) {
    var_dump(current($a));
}

should give 1,1,1. But it gives 2,2,2. After this patch it gave 1,1,1.
But just the other day I downloaded latest source and wrote the test for this, and, unfortunately, it gives 2,3,false.
Why it is so, I don't know, may be something changed in new version.

Well, perhaps, we should close the issue((

@olekukonko

This comment has been minimized.

Show comment
Hide comment
@olekukonko

olekukonko Feb 13, 2013

I don't think it should be closed until its fixed. Has the same issue once it was a mess because it gives different results http://3v4l.org/sBDjl#v430. Can you also look at http://stackoverflow.com/a/14849560/1226894 and confirm how a function affect current output when working with foreach

I don't think it should be closed until its fixed. Has the same issue once it was a mess because it gives different results http://3v4l.org/sBDjl#v430. Can you also look at http://stackoverflow.com/a/14849560/1226894 and confirm how a function affect current output when working with foreach

@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic Feb 13, 2013

Member

@lstrojny What I have was basically a try to improve performance by avoiding the array copy in foreach; the decoupling from the internal array pointer was more of a sideeffect. And as I didn't yet see performance benefits for "real" code (only in artificial benchmarks) I'm not sure we want to do that change.

Member

nikic commented Feb 13, 2013

@lstrojny What I have was basically a try to improve performance by avoiding the array copy in foreach; the decoupling from the internal array pointer was more of a sideeffect. And as I didn't yet see performance benefits for "real" code (only in artificial benchmarks) I'm not sure we want to do that change.

@jpauli

This comment has been minimized.

Show comment
Hide comment
@jpauli

jpauli Jan 20, 2015

Member

Closing as this subject is debatted against PHP7 actually, with a more detailed patch.

Member

jpauli commented Jan 20, 2015

Closing as this subject is debatted against PHP7 actually, with a more detailed patch.

@jpauli jpauli closed this Jan 20, 2015

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