JuliaCollections / DataStructures.jl

Julia implementation of Data structures
https://juliacollections.github.io/DataStructures.jl/latest/
MIT License
679 stars 242 forks source link

Base might export `isfull` in non-breaking release #894

Closed KronosTheLate closed 4 months ago

KronosTheLate commented 5 months ago

I just submitted this PR to julia base, which will be breaking for DataStructures.jl if merged.

In order to maintain compatability with both 1.10 and 1.11, I propose we implement something like the following:

# A function `isfull` was introduced in Julia 1.10, 
# despite being already exported here. The code 
# below makes a conditional definition of `isfull` to 
# maintain simultaneous compatability with 1.10 
# and later versions. Particularly relevant if 1.10 
# indeed becomes next LTS
if isdefined(Base, :isfull)
    @eval DataStructures begin
        # import and extend `Base.isfull`
    end
else
    @eval DataStructures begin
        # insert old definition here
    end
end
vtjnash commented 5 months ago

Can you make that a PR? I think all you need is:

if isdefined(Base, :isfull)
    import Base: isfull
end
CameronBieganek commented 4 months ago

This might qualify as punning. The docstring for Base.isfull is very specific to Channels---it does not provide a generic definition of isfull that could also apply to DataStructures.CircularBuffer.

EDIT: Maybe this comment should have been on PR #896.

KronosTheLate commented 4 months ago

I am not familiar with the concept of "punning", but you do have a point. However I feel as if the meaning of isfull is quite clear from the name, even if it has currently only been deemed usefull for channels in Base.

CameronBieganek commented 4 months ago

"Punning" is using the same generic function in two different contexts with two different meanings. "Is full?" seems clear enough, but it would still need a docstring with a generic definition. However, you have to ask yourself if there are ever any circumstances where it makes sense to write a generic function foo, like this,

function foo(x)
    # ...
    ... isfull(x) ...
    # ....
end

that works on both Channel and DataStructures.CircularBuffer. (I doubt that there are.) If there are no plausible generic use cases like that, then Base.isfull and DataStructures.isfull should probably be separate functions. (In other words, DataStructures should not overload Base.isfull.)

All that being said, the name isfull(c::Channel) seems to be in line with the names of the other functions in Base that operate on channels, so it's probably ok to add Base.isfull. It might have been nice if the Channel functions had been namespaced inside a Channel module, but that ship sailed a long time ago.

vtjnash commented 4 months ago

A CircularBuffer (either bounded or unbounded) would be a reasonable implementation choice for the Channel (single or multi-threaded)

oxinabox commented 4 months ago

Yes. A CircularBuffer is basically the textbook solution to implementing Channels. (I profiled changing the implementation to one back in julia v0.5 days, didn't work out faster) It makes full sense that it would have a very similar API.

CameronBieganek commented 4 months ago

Ok, sorry for the noise.

vtjnash commented 4 months ago

I think technically it is a space savings not a time savings, as you can fit the same sized queue performance in half the memory with a circular buffer implementation

KronosTheLate commented 4 months ago

I will close this up, as the issue is (currently being) delt with by https://github.com/JuliaCollections/DataStructures.jl/pull/896.