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

start working on growable buffer #1472

Closed
wants to merge 13 commits into from
Closed

Conversation

ManasviGoyal
Copy link
Collaborator

No description provided.

@ManasviGoyal ManasviGoyal marked this pull request as draft May 14, 2022 13:44
@ManasviGoyal
Copy link
Collaborator Author

Hi @ianna! I have added a standalone test for Panel implementation using the vector approach. For getitem_at_nowrap(), I have assumed that the size of all panels is same and equal to the number reserved initially. But I have also added the function for the case where the size of each panel is different (it is commented out). There is some issue with the concatenate() function and it is not able to access ptr_ and stops after copying (I think it might be because of the default deleter, not sure). Please check.

Copy link
Collaborator

@ianna ianna left a comment

Choose a reason for hiding this comment

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

The changes to CMakeLists.txt is a temporary one - just to make sure it builds on my computer. The tests run.

initial_ = initial;
ptr_.push_back(std::make_unique<PRIMITIVE>(initial));
ptr_.push_back(std::unique_ptr<PRIMITIVE>(new PRIMITIVE[initial]));
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ManasviGoyal - please, check if the proposed changes work for you. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @ianna. The concatenate function still doesn't work but otherwise the changes work for me. Does it work for you?
Also, should I add any other methods from the original GrowableBuffer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @ianna. The concatenate function still doesn't work but otherwise the changes work for me. Does it work for you?

Yes, please, have a look at the last commit. Also, IMHO we have to move out the index calculations per item. It would be better to make the buffer contiguous first.

Also, should I add any other methods from the original GrowableBuffer?

I think, you need one more method to update the ptr_ with a contiguous panel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, please, have a look at the last commit. Also, IMHO we have to move out the index calculations per item. It would be better to make the buffer contiguous first.

Okay. So will the concatenation be done externally like it is done here in the main() or should I just add it inside the getitem_at_nowrap()?

I think, you need one more method to update the ptr_ with a contiguous panel.

But that can be done by calling concatenate() inside the ptr() method that we already have just like you suggested earlier. So I don't think we need a separate method for that.

@ManasviGoyal ManasviGoyal force-pushed the manasvi/growable_buffer_test branch from 07a44d7 to 0125584 Compare June 8, 2022 02:39
@jpivarski
Copy link
Member

Same question: is this superseded by #1494? I want to know which PRs to pay attention to because they're intended to be merged.

@ManasviGoyal
Copy link
Collaborator Author

Same question: is this superseded by #1494? I want to know which PRs to pay attention to because they're intended to be merged.

Yes. It is no longer needed since we switched to the header only implementation. I'll close this too.

@ManasviGoyal ManasviGoyal deleted the manasvi/growable_buffer_test branch June 30, 2022 13:05
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.

3 participants