Skip to content
This repository has been archived by the owner on Dec 11, 2020. It is now read-only.

Optimize Internet::toAscii() by using a static cache #724

Merged
merged 1 commit into from
Oct 4, 2015

Conversation

vlakoff
Copy link
Contributor

@vlakoff vlakoff commented Oct 4, 2015

It makes quite a difference, even the $transliterationTable array creation is costly.
No possible drawback as it is contained inside the method.

@vlakoff
Copy link
Contributor Author

vlakoff commented Oct 4, 2015

btw, looking at code before #333, it seems this method shouldn't be public.

'ս' => 's','վ' => 'v','տ' => 't','ր' => 'r','ց' => 'ts','փ' => 'p',
'ք' => 'q','և' => 'ev','օ' => 'o','ֆ' => 'f',
);
static $arrayFrom, $arrayTo;
Copy link
Contributor

Choose a reason for hiding this comment

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

please don't define multiple variables on one line

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, wait, ignore me - i thought this was on a different repo...

fzaninotto added a commit that referenced this pull request Oct 4, 2015
Optimize Internet::toAscii() by using a static cache
@fzaninotto fzaninotto merged commit 1003a57 into fzaninotto:master Oct 4, 2015
@fzaninotto
Copy link
Owner

Great, thanks!

@vlakoff
Copy link
Contributor Author

vlakoff commented Oct 4, 2015

So, toAscii() was previously private methods in localized providers, and isn't meant to be used externally (or it shouldn't belong to Internet provider). Shall its visibility be changed to protected?

Also, toAscii() and transliterate() should be moved to bottom of the file to ease reading of the rest.

@fzaninotto
Copy link
Owner

Agreed on both propositions.

@vlakoff vlakoff deleted the transliterate-2 branch October 5, 2015 18:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants