-
Notifications
You must be signed in to change notification settings - Fork 240
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
Make compressor implementations into a plugin via pluginlib #624
Make compressor implementations into a plugin via pluginlib #624
Conversation
{ | ||
return c1 == c2 || std::tolower(c1) == std::tolower(c2); | ||
} | ||
static constexpr const char * name = "rosbag2_compression::BaseCompressorInterface"; |
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.
is there a difference between constexpr const char * name
and constexpr const char name[]
?
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.
yes - the pointer and the array are different C++ types, though I don't understand the details well enough of how they act differently in this situation.
In practice - it builds as char * name
, but not as char name[]
, like so
/usr/bin/ld: librosbag2_compression.so: undefined reference to `rosbag2_compression::CompressionTraits<rosbag2_compression::BaseCompressorInterface>::name'
rosbag2_compression/src/rosbag2_compression/compression_factory_impl.hpp
Outdated
Show resolved
Hide resolved
rosbag2_compression/src/rosbag2_compression/compression_factory_impl.hpp
Show resolved
Hide resolved
rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp
Show resolved
Hide resolved
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.
Left a couple nits, mostly looks good!
rosbag2_compression/include/rosbag2_compression/compression_factory.hpp
Outdated
Show resolved
Hide resolved
rosbag2_compression/src/rosbag2_compression/compression_factory_impl.hpp
Outdated
Show resolved
Hide resolved
…properly in tests Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
610d300
to
33685e9
Compare
Gist: https://gist.githubusercontent.com/emersonknapp/5de8ab86a0473449296fbc19dcf5e60f/raw/c1a41f3fe0dbd1cf61ab98919c12003f79f58659/ros2.repos |
Make compressor implementation into a plugin via pluginlib Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Make compressor implementation into a plugin via pluginlib Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Part of #598
This is incremental progress - the next step will be to create
rosbag2_compression_zstd
package and move the zstd implementation there.Key point: Use the
shared_ptr
implementation ofpluginlib::ClassLoader
instead of eitherunmanagedInstance
oruniqueInstance
. The unique version has a custom deleter, meaning we would have to pass around apluginlib::UniquePtr
- requiring all uses to include the pluginlib headers, which seems a bit restrictive. The unmanaged instance means that pluginlib can't warn us if we destroy the loader before the instance - this can lead to segfaults on theSequentialReader/Writer
destructors if the member variablescompressor_
andcompression_factory_
aren't declared in the right order (which they aren't in the current code). This makes the shared library the most desirable, as it allows pluginlib to clean up after itself while letting the rest of the code.