Skip abstract def warnings after first def with matching parameter names #12167
Conversation
There was a problem hiding this comment.
This is great, thank you! I have just one nitpick suggestion, I'll let you @HertzDevil decide if you prefer to apply it or not
| named_args1 = nil | ||
| named_args2 = nil |
There was a problem hiding this comment.
A nitpick suggestion (see also below)
| named_args1 = nil | |
| named_args2 = nil | |
| named_args1 = [] of String | |
| named_args2 = [] of String |
There was a problem hiding this comment.
It's a performance optimization to avoid the (double) array allocation for the (more likely) case that there are no named args. So I think the current code is probably better (although perhaps not as elegant).
There was a problem hiding this comment.
However, we can know beforehand if there could be an allocation for named args (only if args.size > splat_index).
And I suppose the loop could be refactored and split into different stages:
(0...(splat_index || m1.args.size)).each do |i|
arg1 = m1.args[i]
arg2 = m2.args[i]
return false unless arg1.external_name == arg2.external_name
end
if splat_index
return false unless (m1.args[splat_index].external_name != "") == (m2.args[splat_index].external_name != "")
named_args1 = m1.args[splat_index..].sort!
named_args2 = m2.args[splat_index..].sort!
return false unless named_args1 == named_args2
end
m1.double_splat.nil? == m2.double_splat.nil?I think this should be semantically equivalent (more or less, haven't tested it). And it's definitely more concise. Not sure about performance characteristics, but I doubt it makes a huge difference.
| (named_args1 ||= [] of String) << arg1.external_name | ||
| (named_args2 ||= [] of String) << arg2.external_name |
There was a problem hiding this comment.
| (named_args1 ||= [] of String) << arg1.external_name | |
| (named_args2 ||= [] of String) << arg2.external_name | |
| named_args1 << arg1.external_name | |
| named_args2 << arg2.external_name |
| return false unless arg1.external_name == arg2.external_name | ||
| end | ||
|
|
||
| if named_args1 && named_args2 |
There was a problem hiding this comment.
| if named_args1 && named_args2 | |
| unless named_args1.empty? # named_args1.empty? iff named_args2.empty? |
Fixes #12150.