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

Added ability to offer a buffer range to a publication #401

Merged
merged 3 commits into from
Oct 5, 2017
Merged

Added ability to offer a buffer range to a publication #401

merged 3 commits into from
Oct 5, 2017

Conversation

nicklauslittle
Copy link
Contributor

I wanted the ability to offer multiple buffers and combine them into a single message. I noticed a similar issue had been brought up recently #(394), so I figured I would share what I've done. Comments are welcome.

@nicklauslittle
Copy link
Contributor Author

I had some time today to review how this is implemented in Java and modified the C++ version to map more closely to it.

Another thing I would like some feedback on is that some of the functions could be improved by removing the end iterator as a parameter. Since the length is calculated in the offer function, all the remaining calculations could be based off the length. Would it be beneficial to remove the checks against the end iterator? (Leaving it in will only catch scenarios where the iterator is improperly implemented or where one of the buffers is modified concurrently while being offered.)

@nicklauslittle
Copy link
Contributor Author

There were quite a few conflicts, so I had to rebase. Let me know if there's anything else I need to do to get this pushed through.

@tmontgomery
Copy link
Contributor

Sorry for the delay. I'll look at this tomorrow for a full review. To be honest, not sure I like following the C++ algorithms technique of iterator template params. Not sure it fits here. But I'll review with an open mind. A std::array version feels closer.

@nicklauslittle
Copy link
Contributor Author

No worries on the delay.

My main reason for wanting to use an iterator is because I have a use case involving a linked list of buffers and would prefer to not have to allocate an array on each offer. I figured that an iterator-based approach could cover both the linked list and array use cases. Unfortunately, this is a very C++-specific solution, so the code doesn't match exactly with the Java side of things. But I still tried to make it match up the best that I could.

If you have any questions or comments, or if you want any examples for unit tests or something, let me know. Thanks.

@tmontgomery tmontgomery merged commit f3dc8e5 into real-logic:master Oct 5, 2017
@tmontgomery
Copy link
Contributor

@nicklauslittle thanks! I spent some time exploring this and it does fairly well. It's a more C++-ishy way than using a native array. And probably better overall. Leaving options for usage with the standard algorithms is quite handy. Nice work!

@nicklauslittle
Copy link
Contributor Author

Glad I could help out!

@nicklauslittle nicklauslittle deleted the buffer_range2 branch October 6, 2017 02:16
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