Skip to content

4.0 | Variable sniffs: minor performance tweak #374

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

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Mar 1, 2024

Description

The AbstractVariableSniff by default listens to all T_VARIABLE, T_DOUBLE_QUOTED_STRING and T_HEREDOC tokens.

The majority of the sniffs extending the AbstractVariableSniff, however, only handle T_VARIABLE tokens and in particular, only handle T_VARIABLE tokens when in an OO scope and nothing more.

This small tweak means these sniffs will no longer "listen" to T_DOUBLE_QUOTED_STRING and T_HEREDOC tokens, which should make them marginally faster, in particular for code bases containing lots of T_DOUBLE_QUOTED_STRING and T_HEREDOC tokens.

It also means that these sniff will no longer be triggered for T_VARIABLE tokens outside of an OO context.

Suggested changelog entry

Changed:

  • The following sniffs have received performance related improvements:
    • PEAR.NamingConventions.ValidVariableName *
    • PSR2.Classes.PropertyDeclaration *
    • Squiz.Commenting.VariableComment *
    • Squiz.Scope.MemberVarScope *
    • Squiz.WhiteSpace.MemberVarSpacing *
      • The sniffs marked with a * will no longer listen to non-variable tokens, nor for variables tokens outside of OO context.
        External sniffs which extend one of these sniffs may need adjustment if they want to retain the old behaviour.

@jrfnl jrfnl added this to the 3.9.1 milestone Mar 1, 2024
@jrfnl jrfnl force-pushed the feature/variable-sniffs-minor-performance-tweak branch 2 times, most recently from 8e5fb89 to 580a850 Compare March 5, 2024 09:12
@jrfnl
Copy link
Member Author

jrfnl commented Mar 5, 2024

Rebased without changes. Merging once the build passes.

@jrfnl jrfnl modified the milestones: 3.9.1, 3.10.0 Mar 5, 2024
@jrfnl
Copy link
Member Author

jrfnl commented Mar 5, 2024

On second thought, I will delay merging this PR until the 3.10.0 release as there is a, albeit small, chance that this change could break external sniffs which extend one of the sniffs involved in this PR (as they would inherit the change, which may not be the desired behaviour for their sniff).

@jrfnl jrfnl modified the milestones: 3.10.0, 4.0.0 Apr 22, 2024
@jrfnl jrfnl force-pushed the feature/variable-sniffs-minor-performance-tweak branch from 580a850 to d42dedd Compare April 23, 2024 21:21
@jrfnl jrfnl changed the title Variable sniffs: minor performance tweak 4.0 | Variable sniffs: minor performance tweak Dec 19, 2024
@jrfnl jrfnl force-pushed the feature/variable-sniffs-minor-performance-tweak branch from d42dedd to 70e1b54 Compare April 15, 2025 15:38
@jrfnl jrfnl changed the base branch from master to 4.x April 15, 2025 15:39
The `AbstractVariableSniff` by default listens to all `T_VARIABLE`, `T_DOUBLE_QUOTED_STRING` and `T_HEREDOC` tokens.

The majority of the sniffs extending the `AbstractVariableSniff`, however, only handle `T_VARIABLE` tokens and in particular, only handle `T_VARIABLE` tokens when in an OO scope and nothing more.

This small tweak means these sniffs will no longer "listen" to `T_DOUBLE_QUOTED_STRING` and `T_HEREDOC` tokens, which should make them marginally faster, in particular for code bases containing lots of `T_DOUBLE_QUOTED_STRING` and `T_HEREDOC` tokens.

It also means that these sniff will no longer be triggered for `T_VARIABLE` tokens outside of an OO context.
@jrfnl jrfnl force-pushed the feature/variable-sniffs-minor-performance-tweak branch from 70e1b54 to c403015 Compare April 15, 2025 15:40
@jrfnl jrfnl merged commit 118f141 into 4.x Apr 15, 2025
52 checks passed
@jrfnl jrfnl deleted the feature/variable-sniffs-minor-performance-tweak branch April 15, 2025 18:34
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.

2 participants