Skip to content

Fix #79364: When copy empty array, next key is unspecified #5253

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

Closed
wants to merge 1 commit into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Mar 10, 2020

We must not forget to keep the nNextFreeElement when duplicating
empty arrays.

We must not forget to keep the `nNextFreeElement` when duplicating
empty arrays.
@nikic
Copy link
Member

nikic commented Mar 10, 2020

I believe the correct behavior is the other way around: We should always reset the next free element when copying arrays.

@nikic
Copy link
Member

nikic commented Mar 10, 2020

Or maybe not, who knows. Is there a benefit for going in one direction or the other?

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

As we do copy the internal array pointer, which is a very similar bit of hidden state, I guess this is okay.

For master, we should maybe investigate how expensive it is to have an "always correct" next free element...

@cmb69
Copy link
Member Author

cmb69 commented Mar 11, 2020

Thanks! Applied as 2462f2d.

For master, we should maybe investigate how expensive it is to have an "always correct" next free element...

Well, I think the current behavior (modulo this bug, which is about a very special case, namely empty arrays with nNextFreeElement != 0) is quite desireable, because a copy (of an array) should be independent from the original. E.g. https://3v4l.org/872qn should always produce the same output.

@cmb69 cmb69 closed this Mar 11, 2020
@cmb69 cmb69 deleted the cmb/79364 branch March 11, 2020 08:13
@nikic
Copy link
Member

nikic commented Mar 11, 2020

@cmb69 Sorry, I was a bit unclear there: What I meant by "always correct" is to adjust the next free element on unset, so it actually stays the next free element, without holes.

@cmb69
Copy link
Member Author

cmb69 commented Mar 11, 2020

Ah, thanks! I agree that this is something worth to be investigated. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants