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

Improve ReflectionEnum->getCases() performance #1410

Open
wants to merge 8 commits into
base: 6.30.x
Choose a base branch
from
Open

Improve ReflectionEnum->getCases() performance #1410

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
72a45e4
Improve ReflectionEnum->getCases() performance
staabm Mar 22, 2024
7aa8054
Remove unnecessary array_values()
staabm Mar 22, 2024
a7ff65c
memoize cases in ReflectionEnum
staabm Mar 22, 2024
7948bc2
pull out repeated isBacked() of the loop
staabm Mar 22, 2024
12a5789
Implement Memoize<T>
staabm Mar 22, 2024
cf29bb0
Discard function after use
staabm Mar 22, 2024
646e4e1
incorporate feedback
staabm Mar 22, 2024
3de5a55
missing return
staabm Mar 22, 2024
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
14 changes: 9 additions & 5 deletions src/Reflection/Adapter/ReflectionEnum.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,14 +530,18 @@ public function getCase(string $name): ReflectionEnumUnitCase|ReflectionEnumBack
/** @return list<ReflectionEnumUnitCase|ReflectionEnumBackedCase> */
Copy link
Member

Choose a reason for hiding this comment

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

Consideration here: we perhaps return list<ReflectionEnumUnitCase>|list<ReflectionEnumBackedCase> rather than list<ReflectionEnumUnitCase|ReflectionEnumBackedCase> 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK phpstan is not able to differentiate it, but I can do it if you prefer

Copy link
Member

Choose a reason for hiding this comment

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

It's more precise for the consumer regardless, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. I already changed it silently :)

public function getCases(): array
{
/** @psalm-suppress ImpureFunctionCall */
return array_map(function (BetterReflectionEnumCase $case): ReflectionEnumUnitCase|ReflectionEnumBackedCase {
$cases = $this->betterReflectionEnum->getCases();

$mappedCases = [];
foreach ($cases as $case) {
if ($this->betterReflectionEnum->isBacked()) {
Copy link
Member

@Ocramius Ocramius Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This method call is repeated in the loop: let's pull it out of here

return new ReflectionEnumBackedCase($case);
$mappedCases[] = new ReflectionEnumBackedCase($case);
} else {
$mappedCases[] = new ReflectionEnumUnitCase($case);
}
}

return new ReflectionEnumUnitCase($case);
}, array_values($this->betterReflectionEnum->getCases()));
return $mappedCases;
}

public function isBacked(): bool
Expand Down
Loading