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

Adding Deques #292

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Adding Deques #292

wants to merge 4 commits into from

Conversation

Divesh-Otwani
Copy link
Contributor

I wanted to build out a few data structures that are relatively easy to get out. This is one of them.

@Divesh-Otwani Divesh-Otwani self-assigned this Jan 18, 2021
@Divesh-Otwani Divesh-Otwani removed the request for review from utdemir January 18, 2021 21:18
Copy link
Contributor

@utdemir utdemir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were late removing me as a reviewer, now I am aware of this and had to comment :).

I like the idea of having a few more data structures. I have some comments about the implementation and the interface; also some tests would be good.

src/Data/Deque/Mutable/Linear.hs Outdated Show resolved Hide resolved
--
-- It is designed to be imported qualfied:
--
-- > import qualfied Data.Deque.Mutable.Linear as Deque
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is crucial to mention that this is a bounded deque. I would even call it Data.Dequeue.Bounded.Mutable.Linear instad. It would also be good to mention:

  • That the underlying storage is an array
  • When full, newer elements overwrite the older elements

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't overwrite. It crashes if you try to fill it after it's full. I think that makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am having a hard time thinking when would I require a queue where I certainly know that it will contain less than some number of elements. I can think of cases where it's okay to be forgetful, or don't accept the new element, or in concurrent cases (which is not the case in this code) it should block; but in all cases I can think of there is no guarantee that the queue is not empty or it is not full.

Even if we go with the crashy version, we should still mention that it is a bounded queue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with adding comments saying it is bounded. But ...

Do you think it's just better for me to use array resizing internally and have an unbounded structure to begin with? I feel like your argument makes sense and I can't think of a case where we want a bounded queue or stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this ^, done.

src/Data/Deque/Mutable/Linear.hs Outdated Show resolved Hide resolved
src/Data/Deque/Mutable/Linear.hs Outdated Show resolved Hide resolved
src/Data/Deque/Mutable/Linear.hs Outdated Show resolved Hide resolved
src/Data/Deque/Mutable/Linear.hs Outdated Show resolved Hide resolved
src/Data/Deque/Mutable/Linear.hs Outdated Show resolved Hide resolved
@Divesh-Otwani Divesh-Otwani reopened this Jan 19, 2021
@Divesh-Otwani
Copy link
Contributor Author

Just going to comment that I don't know how I closed this a moment ago. I think I accidentally used a strange key-sequence or something that caused that.

@utdemir
Copy link
Contributor

utdemir commented May 12, 2021

Sorry for the radio silence. For now I think we should hold off merging this PR (alongside with other new collection PR's #293 and #295), because we already have some performance issues with our existing linear collections; and having even more collections built on top of them would not solve that.

Once we get their performance right, we should take another look at these PR's, so we can merge them more confidently of their usefulness.

@Divesh-Otwani
Copy link
Contributor Author

Dude I agree. Let's do things right and try to only keep solid stuff in the codebase.

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

Successfully merging this pull request may close these issues.

2 participants