Skip to content
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

Base might export isfull in non-breaking release #894

Closed
KronosTheLate opened this issue Feb 2, 2024 · 9 comments
Closed

Base might export isfull in non-breaking release #894

KronosTheLate opened this issue Feb 2, 2024 · 9 comments

Comments

@KronosTheLate
Copy link
Contributor

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
Copy link
Contributor

vtjnash commented Feb 4, 2024

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

if isdefined(Base, :isfull)
    import Base: isfull
end

@CameronBieganek
Copy link

CameronBieganek commented Feb 10, 2024

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
Copy link
Contributor Author

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
Copy link

CameronBieganek commented Feb 13, 2024

"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
Copy link
Contributor

vtjnash commented Feb 13, 2024

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

@oxinabox
Copy link
Member

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
Copy link

Ok, sorry for the noise.

@vtjnash
Copy link
Contributor

vtjnash commented Feb 14, 2024

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
Copy link
Contributor Author

I will close this up, as the issue is (currently being) delt with by #896.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants