-
-
Notifications
You must be signed in to change notification settings - Fork 81
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
Add Performance::NumericPredicate cop #440
base: master
Are you sure you want to change the base?
Conversation
aff4286
to
7bdfdef
Compare
Performance::NumericPredicate cop identifies places where numeric uses predicates like `positive?`, `negative?` and for some cases `zero?` should be converted to compare operator. The `Performance::NumericPredicate` cop is added to identify instances where numeric predicates such as `positive?`, `negative?`, and occasionally `zero?` should be replaced with comparison operators for improved efficiency. Predicates incur a performance overhead by executing a method before comparison. A small benchmark comparison between using a comparison operator (`> 0`) and `positive?` illustrates the performance difference: ```ruby x.report("compare with 0") { arr.each {|i| i > 0 } } x.report("positive?") { arr.each {|i| i.positive? } } ``` Benchmark results on Ruby 3.3.0 (with YJIT) indicate a significant performance gain when using the comparison operator: ``` ruby 3.3.0 (2023-12-25 revision 5124f9ac75) +YJIT [arm64-darwin23] Warming up -------------------------------------- compare with 0 1.000 i/100ms positive? 1.000 i/100ms Calculating ------------------------------------- compare with 0 3.153 (± 0.0%) i/s - 95.000 in 30.132600s positive? 2.397 (± 0.0%) i/s - 72.000 in 30.042688s Comparison: compare with 0: 3.2 i/s positive?: 2.4 i/s - 1.32x slower ``` This cop is unsafe because it cannot be guaranteed that the receiver is Number and could be noisy. Signed-off-by: Michael Nikitochkin <[email protected]>
7bdfdef
to
795f7cc
Compare
REPLACEMENTS = { negative?: '<', positive?: '>', zero?: '==' }.freeze | ||
|
||
def_node_matcher :num_predicate?, <<~PATTERN | ||
(send $numeric_type? ${:negative? :positive? :zero?}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also look for nonzero?
This is just |
@Earlopain Are there any established practices for handling conflicts between rubocop cops and performance cops? |
Performance::NumericPredicate cop identifies places where numeric uses predicates like
positive?
,negative?
and for some caseszero?
should be converted to compare operator.The
Performance::NumericPredicate
cop is added to identify instances where numeric predicates such aspositive?
,negative?
, and occasionallyzero?
should be replaced with comparison operators for improved efficiency.Predicates incur a performance overhead by executing a method before comparison. A small benchmark comparison between using a comparison operator (
> 0
) andpositive?
illustrates the performance difference:Benchmark results on Ruby 3.3.0 (with YJIT) indicate a significant performance gain when using the comparison operator:
This cop is unsafe because it cannot be guaranteed that the receiver is Number and could be noisy.
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.