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

Issues with Performance/Casecmp #2614

Closed
deivid-rodriguez opened this issue Jan 9, 2016 · 3 comments
Closed

Issues with Performance/Casecmp #2614

deivid-rodriguez opened this issue Jan 9, 2016 · 3 comments

Comments

@deivid-rodriguez
Copy link
Contributor

Hi! Two issues with this cop:

  • It autocorrects wrong. 'HOLA'.downcase == 'hola' gets autocorrected to 'HOLA'.casecmp('hola'), but the first version returns a boolean and the second an integer.
  • I don't think it should be enabled by default. The correct autocorrected version would be 'HOLA'.casecmp('hola') == 0 which is arguably less readable. I think performance cops should only be enabled by defaulf if they don't harm readability.
deivid-rodriguez pushed a commit to deivid-rodriguez/nipanipa that referenced this issue Jan 9, 2016
@segiddins
Copy link
Contributor

What if it autocorrected to 'HOLA'.casecmp('hola').zero? ?

@deivid-rodriguez
Copy link
Contributor Author

I still find it less readable, but it's not up to me... :)

@alexdowad
Copy link
Contributor

@segiddins, since this cop is for "performance", could you benchmark the zero? fix and see if it still buys any performance?

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

No branches or pull requests

3 participants