Skip to content

Fix nullsafe operator on delayed oplines #5994

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

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Aug 15, 2020

No description provided.

@iluuu1994 iluuu1994 force-pushed the nullsafe-on-delayed-oplines branch from 7a4e7d5 to 491caf6 Compare August 15, 2020 16:01
@cmb69 cmb69 added the Bug label Aug 21, 2020
@cmb69
Copy link
Member

cmb69 commented Aug 21, 2020

This fixes https://bugs.php.net/79995.

@iluuu1994
Copy link
Member Author

@nikic I'd appreciate a review. I'm not sure if there's a better solution that doesn't require tracking the delayed offsets.

@iluuu1994 iluuu1994 force-pushed the nullsafe-on-delayed-oplines branch 2 times, most recently from 37b0c2f to 2460728 Compare August 24, 2020 10:52
@iluuu1994
Copy link
Member Author

I just realized tracking the entire stack was absolutely unnecessary. We're now only storing the last offset. Hopefully this is fine now.

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.

I don't think the last offset is sufficient. Consider something like this:

<?php

$foo = (object) ['bar' => 0];
$array = [[null]];
var_dump($array[0][$foo->bar]?->baz);

You don't want the offset from the $foo->bar.

@iluuu1994
Copy link
Member Author

I don't think the last offset is sufficient. Consider something like this:

😕 You're right. I'll take another look after work.

@iluuu1994 iluuu1994 force-pushed the nullsafe-on-delayed-oplines branch from 2460728 to 473beb0 Compare August 26, 2020 22:19
@iluuu1994
Copy link
Member Author

That was way harder than it should've been. It works now, let me clean it up tomorrow.

@iluuu1994 iluuu1994 force-pushed the nullsafe-on-delayed-oplines branch from 473beb0 to 369c2ea Compare August 30, 2020 10:12
@iluuu1994
Copy link
Member Author

Sorry for not getting back to this sooner. Can you give this another review? Can you think of a simpler way to solve this?

Also, maybe we should rename all the short_circuiting stuff to jmp_null_short_circuiting because jmp_null is certainly not the only thing that does short circuiting.

@nikic
Copy link
Member

nikic commented Aug 31, 2020

I've opened #6056 with another approach that seems more straightforward to me. What do you think about it?

@iluuu1994
Copy link
Member Author

Closing in favor of #6056.

@iluuu1994 iluuu1994 closed this Aug 31, 2020
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.

4 participants