Skip to content

Conversation

@TysonAndre
Copy link
Contributor

Grammar test suite sped up from 1.2 seconds to 1.1 seconds.

This went from O(n) (loop over (around half of) 183 constants) to O(1) (hash lookup)

@msftclas
Copy link

@TysonAndre,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

src/Token.php Outdated
return $mapToKind;
}

/** @return int (Or string, if $kindName wasn't found) */
Copy link
Member

Choose a reason for hiding this comment

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

returns string, right?

src/Token.php Outdated
/** @return int (Or string, if $kindName wasn't found) */
public static function getTokenKindNameFromValue($kindName) {
$mapToKind = self::getTokenKindNameFromValueMap();
return $mapToKind[$kindName] ?? $kindName;
Copy link
Member

Choose a reason for hiding this comment

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

The variable names are confusing here (and they were like that before). $kindName the parameter is really the value? And I think it should return "" or null if it isn't found.

Copy link
Contributor Author

@TysonAndre TysonAndre Sep 20, 2017

Choose a reason for hiding this comment

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

Was in a rush in the morning, noticed that $kindName was wrong but didn't fix it (Threw the rest of the documentation it was based off of off).

This appears to be used in generating debug data from the dumped values in tests, and I don't plan on changing the function behavior in this PR. (Having an unknown int is more useful than null if there actually was a bug)

Copy link
Member

Choose a reason for hiding this comment

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

Sure. Thanks!

Grammar test suite sped up from 1.2 seconds to 1.1 seconds.

This went from O(n) (loop over 183 constants) to O(1) (hash lookup)
@roblourens roblourens merged commit 099d0b1 into microsoft:master Sep 20, 2017
@TysonAndre TysonAndre deleted the speed-up-token branch June 19, 2018 02:15
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.

3 participants