Remove delete_if

[caspiano]

Implementing changes referenced by @asterite in #9856

• Deprecates Hash#delete_if
• Removes Deque#delete_if
• Adds Deque#reject!
• Adds Deque#select!

[straight-shoota]

Let's first have a proper discussion on how to proceed with this. Please open an issue and describe your proposal. Only when that's come to a conclusion, it's time to implement it. Proposals tend to change over time. Starting a discussion whit a PR (or rather jumping from one PR to another) has proven to be an inefficient means to communicate about proposed changes.

[caspiano]

Sorry @straight-shoota, I was jumping on work mentioned in the discussion here #9856, specifically this comment

[asterite]

Thank you!

[jkthorne]

Even though we are not 1.0 should there be a depreciation warning for this?

[jzakiya]

Instead of using % to check for even|odd, as below:

a2 = a1.select! { |elem| elem % 2 == 0 }

its faster to do:

a2 = a1.select! { |elem| elem & 1 == 0 }

or

a2 = a1.select! { |elem| (elem & 1).zero? }

I see this kind of unnecessary|expensive use of % throughout the codebase to test for even|odd.
Using this simpler|faster test can increase observable performance, especially if it's within loops, which they typically are.

[Blacksmoke16]

@jzakiya Why not just a1.select! &.even?.

[jzakiya]

I was pointing out how to improve that code snippet by eliminating using %, with minimal change.

I suspect many (younger) programmers don't really understand even|odd of an integer is merely if the lsb is 1 or 0.

I grew up as a hardware engineer, and this is what you do in hardware, branch on the lsb value.

Yes, you can (should?) use .even? or .odd? (assuming they're implemented optimally) for this type of testing,
but that will have to be promoted (educated) to users for them to understand how to best code in Crystal.

[j8r]

An option can be to have a 0.36.0 before 1.0 in order to have the deprecation warnings. After that, we will have a clean, 0 deprecations for 1.0.

The deprecation warnings are important to make the Crystal upgrade experience a breeze @wontruefree . I agree with you that it doesn't feel right to have warnings for a "definitive" 1.0 API.

[asterite]

@j8r the next version is 1.0, that's not discussable

[stakach]

@jzakiya LLVM should optimise that operation in release and as elem % 2 == 0 is arguably more readable than elem & 1 == 0 its probably worth leaving it

http://duriansoftware.com/joe/Optimizing-is-multiple-checks-with-modular-arithmetic.html

[caspiano]

@jzakiya I copied the test from Array's test suite, I think the most crystalline solution for this would be using a1.select! &.even? and a1.reject! &.even?. I can update that now.
Perhaps you could open another PR for other occurrances that you've seen across the codebase?

[jzakiya]

Thank you @caspiano, and I agree they are the clearest for someone reading the code.

I am willing (with others) to do a thorough (as possible that I can do) review of Crystal's codebase to identify instances to use &.even? and &.odd? if the devs indicate they are willing to take PRs to change these instances.

Or I (and others) can first identify such instances to log for change for future consideration.
Let me know which is preferable (make a log of instances first, or submit PRs to change first).
Also, what level of code granularity do you want this done on?
Should it be of a file basis, module, class, method by method, etc.

While writing this, I think it would be more manageable to create a list of instances first, so people can review them to insure changing their semantics won't create unexpected edge cases for their particular use cases, just to be careful.

Addition:
I just realized searching the codebase can be automated, which would make the identification|logging task quite easy.

[asterite]

You won't find such instances except in tests, and there it doesn't matter.

[caspiano]

@bcardiff following from your comment here

From a manual code search on github, I saw around ~80 instances of Hash#delete_if and almost no instances of Deque#delete_if.
IMO, the way forward would be deprecation of Hash#delete_if and an immediate bypass of deprecation for Deque#delete_if.

[jzakiya]

So @asterite for clarity, you don't favor even changing test code?

I think even changing test code instances is worth doing, because you would be reinforcing best practices to people reviewing the codebase, while reducing its size too.

[caspiano]

@jzakiya updated. I doubt anyone's going to stop you improving the tests. However this is best discussed on a seperate PR.

[caspiano]

@j8r @wontruefree @bcardiff

Reverted removal of Hash#delete_if and added a deprecation notice.

[jzakiya]

@jzakiya updated. I doubt anyone's going to stop you improving the tests. However this is best discussed on a seperate PR.

Will do.

[bcardiff]

Let's make return self or nil as in Hash.

Ref: #9856 (comment)

[straight-shoota]

I'd prefer to change Hash#reject! to be non-nilable instead. There's really not much reason to return nil and it breaks method chaining. If you need to know whether an item was removed, you can keep track of whether the block ever returns false as demonstrated in #9879 (comment)
That clearly communicates what's going on and avoids having to do some nasty nil-checks when chaning calls. I doubt there are many use cases for the nilable return type anyway.

[bcardiff]

Changing Hash#reject! to return always self would be a silent breaking-change.

Ruby returns self or nil on {Array, Hash}#reject!.

tap can be used to perform chaining if wanted. c.tap(&.reject!(...)).foo

[straight-shoota]

Changing Hash#reject! to return always self would be a silent breaking-change.

Yes, that's unfortunate. We don't necessarily have to change it, at least not right now. But I would not introduce the same weird return type for new methods.

tap can be used to perform chaining if wanted. c.tap(&.reject!(...)).foo

This clearly shows that it doesn't make any sense to have #reject! return self as an indicator for an item being deleted. Returning Bool would be sufficient if such a return value is desired. Otherwise it should just be self always.
Even nil always is better than self | nil.

[caspiano]

Should we then update Array#reject! to also return self | nil?
I don't see a particularly good reason for the inconsistency between these interfaces besides least astonishment coming from ruby. I think the return type of self is better purely for the benefit of simply chaining methods.

[bcardiff]

Ok, let's keep reject! : self then. We will change the hash#reject! in another PR.... I hope this breaking-change will not haunt us later...