-
Notifications
You must be signed in to change notification settings - Fork 215
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
Add support for constructing Buffer from std::array #138
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Minor comment.
include/cppkafka/buffer.h
Outdated
*/ | ||
template <typename T, size_t N> | ||
Buffer(const std::array<T, N>& data) | ||
: data_(reinterpret_cast<const DataType*>(&data[0])), size_(N * sizeof(T)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: can you use data.data()
and data.size()
rather than &data[0]
and N * sizeof(T)
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do! first time working with std::arrays
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, that works! One more thing: can you add a deleted constructor taking a std::array<T, N>&&
just like there is for vector
and string
? e.g. this one. This will make sure people can't misuse Buffer
and construct it from a temporary array
, which will end up making the buffer contain garbage once it goes out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be fixed
Maybe we can also have an overload for raw arrays |
Yeah, that's a good idea! |
@accelerated is there a difference between construction with the raw array and the constructor Buffer(const T*, size_t size) here: None of the code in message_builder.h uses that template from line 76, so maybe the solution is to overload payload() of BasicMessageBuilder with payload(const T* value, size_t size), and also overload the implicit conversion between ConcreteMB and MBuilder with something that will construct the buffer of the MBuilder with that same pointer and size to that no data needs to be copied. |
@tgaldes, the difference is simplicity and easy of use. Just like you can skip the |
Thanks! The PR build is flapping but it's unrelated. |
@accelerated Here's the addition you requested: #140 |
No description provided.