Changed 'count == 0' to 'isEmpty' and add 'final' keyword to class #401
Conversation
quokkaKyu
changed the title
lorentey
left a comment
lorentey
left a comment
There was a problem hiding this comment.
Thanks for this work! I do not think it is a good idea to start contributing to a project by applying large-scale stylistic changes.
-
Please retarget this PR on the
release/1.1branch. Thefuturebranch is for experimentation; it isn't intended to ever ship in any production release. -
Marking storage classes final is a good idea. Let's do it! The
_DequeBufferchange is acceptable, and we should land it. (I expect the compiler infersfinalin this case anyway, but making it explicit is not going to hurt.) -
On the other hand, I am not interested in microoptimizing XCTestCase declarations. Please remove the changes that mark those classes final.
-
I do not expect
count == 0vsisEmptyto make any actual difference for the specific types on which we're calling them. Do you have any evidence to the contrary? -
Please make sure not to break indentation.
| #endif | ||
|
|
||
| class CombinatoricsTests: CollectionTestCase { | ||
| final class CombinatoricsTests: CollectionTestCase { |
There was a problem hiding this comment.
I'm not interested in microoptimizing XCTestCase classes. Please remove these changes from all test files.
There was a problem hiding this comment.
Thank you for the reply. i will delete the changes in the test files.
| assert(offsets.lowerBound >= 0 && offsets.upperBound <= count) | ||
| let lower = slot(forOffset: offsets.lowerBound) | ||
| let upper = slot(forOffset: offsets.upperBound) | ||
| if offsets.count == 0 || lower < upper { |
There was a problem hiding this comment.
I don't expect Range.count == 0 to produce less efficient code than Range.isEmpty -- do you have proof that it matters?
There was a problem hiding this comment.
No, I may be mistaken. Since it is a RandomAccessCollection type, I don’t think there will be any difference.
| let lower = slot(forOffset: offsets.lowerBound) | ||
| let upper = slot(forOffset: offsets.upperBound) | ||
| if offsets.count == 0 || lower < upper { | ||
| return .init(start: ptr(at: lower), count: offsets.count) |
There was a problem hiding this comment.
Please do not break indentation.
There was a problem hiding this comment.
yes, i won't break the indentation.
| count: 1 | ||
| ) { children, items in | ||
| assert(items.count == 1 && children.count == 0) | ||
| assert(items.count == 1 && children.isEmpty) |
There was a problem hiding this comment.
children is of type UnsafeMutableBufferPointer. Do you have evidence that this change has a meaningful impact?
There was a problem hiding this comment.
No, I may be mistaken. Since it is a RandomAccessCollection type, I don’t think there will be any difference.
|
Closing; the |
2 participants
Hello, when checking whether the value of Collection Types is empty, how about using 'isEmpty' instead of 'count == 0' considering time complexity and readability?
And How about adding the final keyword to classes that do not inherit?
Thanks for reading.
Checklist