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.

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

Refactor: constant array type inference #6279

Merged
merged 9 commits into from Aug 11, 2021

Conversation

weirdan
Copy link
Collaborator

@weirdan weirdan commented Aug 10, 2021

This brings constant array type inference closer to non-constant analysis, and is a step toward fixing #6263

@weirdan weirdan requested a review from orklah August 10, 2021 22:21
Copy link
Collaborator

@orklah orklah left a comment

Choose a reason for hiding this comment

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

Seems great. Could there be an opportunity to mutualize the code here with what's in ArrayAnalyzer?

);
}

// if this array looks like an object-like array, let's return that instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a similar check could be done for inferring TList?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean, for cases where it crossed the threshold for can_create_objectlike?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw multiple cases where can_create_objectlike can be false, so any of those I guess

);
}
} else {
$array_creation_info->item_key_atomic_types[] = new Type\Atomic\TInt();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could probably directly put a TLiteralInt with $int_offset in here I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps. That wouldn't be a refactoring in its strict sense though 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

fair enough :)

@weirdan
Copy link
Collaborator Author

weirdan commented Aug 11, 2021

Could there be an opportunity to mutualize the code here with what's in ArrayAnalyzer?

To share? I thought so, e.g. handleUnpackedArray() could technically be shared, at a cost of some additional checks. But there is sufficient number of differences and a lot of what's possible in dynamic array just cannot happen in static arrays. So I think we'll just end up making those two parts similar, but separate.

@weirdan weirdan merged commit 7c339c1 into vimeo:master Aug 11, 2021
@weirdan weirdan deleted the refactor-constant-array-inference branch August 11, 2021 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants