Remove pybind11 dependency from cpp library #40
Conversation
liambenson
requested review from
jaredjxyz,
kashish-jain,
johnembark,
brendangeck,
sanjeevanil-embark and
taehoon-embark
lib/ros_value.h
Outdated
| namespace pybind11 { | ||
| struct buffer_info; | ||
| } | ||
|
|
There was a problem hiding this comment.
Hmm forward declarations can be a dangerous hole to go down - Our style guide says to avoid them whenever possible https://google.github.io/styleguide/cppguide.html#Forward_Declarations
Why do you need the forward declaration here? Can you structure this in a way that doesn't need it?
There was a problem hiding this comment.
Agreed! Since this is a 3rd party library, it might just be better to not have a forward declaration. Do you need RosValue::getPrimitiveArrayBufferInfo() declaration in this file @liamembark? If not, then we can delete this forward declaration, I think.
There was a problem hiding this comment.
Ya so the underlying need for this is that the buffer_info object needs access to the underlying memory buffer, but this is a private member.
As a result, whatever is going to access this memory buffer needs to be declared along with the RosValue.
AFAIK this is always going to require forward declaration (if not let me know), but one alternative to forward declaring a piece of a third party library, I could instead forward declare a friend function that will be defined in the python library to provide access!
There was a problem hiding this comment.
Liam I'll DM you to discuss this a bit, I think I understand the problem but want to dig in just a little more
Fixes: DATA-622
Description of Changes
The library shouldn't be dependent on pybind11, only the python bindings should.
Test Plan
Unit tests.