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

NamespacesAnalyzer - Optimize performance #3877

Merged
merged 1 commit into from Jul 6, 2018

Conversation

stof
Copy link
Contributor

@stof stof commented Jul 5, 2018

Instead of looping on all tokens, even inside the namespace it identified, the analyzer now continues the analysis after the end of the identified namespace, thanks to the fact that namespaces cannot be nested.

When implementing #3876, I compared the NamespacesAnalyzer implementation with \PhpCsFixer\Fixer\FunctionNotation\NativeFunctionInvocationFixer::getUserDefinedNamespaces. The NamespacesAnalyzer also identifies the namespace name instead of skipping over it (and returns the global namespace too). But I found out that NativeFunctionInvocationFixer was more optimized, as it was not re-analyzing all tokens inside a namespace. This makes NamespacesAnalyzer implement the same optimization.

Instead of looping on all tokens, even inside the namespace it identified,
the analyzer now continues the analysis after the end of the identified
namespace, thanks to the fact that namespaces cannot be nested.
@keradus
Copy link
Member

keradus commented Jul 6, 2018

We do push code refactoring to oldest maintained branch, I have changed the target and recreated your branch @stof (force-push)

@keradus keradus added this to the 2.12.2 milestone Jul 6, 2018
@keradus keradus changed the title Optimize the namespaces analyzer NamespacesAnalyzer - Optimize performance Jul 6, 2018
@keradus
Copy link
Member

keradus commented Jul 6, 2018

Thank you @stof.

@keradus keradus merged commit 8321c26 into PHP-CS-Fixer:2.12 Jul 6, 2018
keradus added a commit that referenced this pull request Jul 6, 2018
This PR was merged into the 2.12 branch.

Discussion
----------

NamespacesAnalyzer - Optimize performance

Instead of looping on all tokens, even inside the namespace it identified, the analyzer now continues the analysis after the end of the identified namespace, thanks to the fact that namespaces cannot be nested.

When implementing #3876, I compared the NamespacesAnalyzer implementation with `\PhpCsFixer\Fixer\FunctionNotation\NativeFunctionInvocationFixer::getUserDefinedNamespaces`. The NamespacesAnalyzer also identifies the namespace name instead of skipping over it (and returns the global namespace too). But I found out that NativeFunctionInvocationFixer was more optimized, as it was not re-analyzing all tokens inside a namespace. This makes NamespacesAnalyzer implement the same optimization.

Commits
-------

8321c26 Optimize the namespaces analyzer
keradus added a commit that referenced this pull request Jul 6, 2018
…r to remove duplicated code (stof)

This PR was merged into the 2.12 branch.

Discussion
----------

NativeFunctionInvocationFixer - use the NamespacesAnalyzer to remove duplicated code

Instead of maintaining 2 different analyzers to find the beginning and the end of a namespace block, this reuses the existing NamespacesAnalyzer.

See #3877 which optimizes it based on existing optimization in the implementation I'm deleting here.

Commits
-------

b16c2f0 Use the NamespacesAnalyzer in the NativeFunctionInvocationFixer
@stof stof deleted the namespace_analysis branch July 6, 2018 10:04
@stof
Copy link
Contributor Author

stof commented Jul 6, 2018

We do push code refactoring to oldest maintained branch,

I was not aware of that. Good to know.

@keradus
Copy link
Member

keradus commented Jul 6, 2018

for next MINOR branch we do push only what MINOR is reserved for - features ;)
all the rest, like bugs, docs, typos, refactoring - to lowest possible branch.

the more similar codebase between versions, the easier to maintain

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

Successfully merging this pull request may close these issues.

None yet

2 participants