storage interfaces #19
Conversation
There was a problem hiding this comment.
Regarding the naming of the storage_interface:: classes: for me using e.g. 'storage_interface::ReadOnlyinstead ofstorage_interface::ReadOnlyInterface` would also be fine.
We will also need a function StorageFactory::open_for_reading(uri, io_flags) that does not require to specify the storage plugin. But this should be left for a followup pr.
| public: | ||
| virtual ~BaseIOInterface() = default; | ||
| virtual void open(const std::string & uri, IOFlag io_flag) = 0; | ||
| virtual bool is_open() const = 0; |
There was a problem hiding this comment.
There is no need for is_open and close when we do the closing in the destructor (more RAII style, we need the open due to pluginlib limitations. Ideally we would also do the opening in the constructor but we cannot have constructor arguments)
| { | ||
| public: | ||
| virtual ~BaseReadInterface() = default; | ||
| virtual std::shared_ptr<SerializedBagMessage> read_next() = 0; |
There was a problem hiding this comment.
I guess we need a virtual bool has_next() to go with read_next.
| template<> | ||
| struct StorageTraits<storage_interfaces::ReadWriteInterface> | ||
| { | ||
| static constexpr storage_interfaces::IOFlag io_flag = storage_interfaces::IOFlag::READ_WRITE; |
There was a problem hiding this comment.
I can see no reason for having the io_flag member of StorageTraits. Same applies the the ReadOnlyInterface below.
| } | ||
|
|
||
| try { | ||
| instance->open(uri, flag); |
There was a problem hiding this comment.
The opening of the storage should not happen int get_interface_instance but in the factory below. This also removes the need to have the uri argument.
| { | ||
| public: | ||
| virtual ~ReadWriteInterface() = default; | ||
| void open(const std::string & uri, IOFlag io_flag = IOFlag::READ_WRITE) override = 0; |
There was a problem hiding this comment.
I would prefer to have no default arguments here (also for open in ReadOnlyInterface) as the value for virtual functions can be surprising (static type is relevant). There is also no real advantage as open will only be called by the storage factory where we can handle all complexities of calling open.
| std::shared_ptr<ReadableStorage> get_read_only_storage( | ||
| ~StorageFactoryImpl() = default; | ||
|
|
||
| std::shared_ptr<ReadWriteInterface> get_read_write_storage( |
There was a problem hiding this comment.
I would prefer open_for_writing as method name to emphasize that the factory returns an already opened storage. Regarding the arguments we should add an io_flags argument (possible with the appropriate default).
The call to open should happen here (see also above comment).
| private: | ||
| bool storage_id_is_present( | ||
| std::vector<std::string> registered_classes, const std::string & storage_id) | ||
| std::shared_ptr<ReadOnlyInterface> get_read_only_storage( |
There was a problem hiding this comment.
Similar to the above comment I would prefer open_for_reading as method name and an additional argument io_flags.
Method body: the io_flag and uri would be used in the open call instead of the get_interface_instance (see also my above comment).
|
|
||
| StorageFactory::~StorageFactory() | ||
| { | ||
| delete impl_; | ||
| } | ||
|
|
||
| std::shared_ptr<ReadWriteStorage> StorageFactory::get_read_write_storage( | ||
| ReadWriteStorageSharedPtr StorageFactory::get_read_write_storage( |
There was a problem hiding this comment.
My above comment regarding the names open_for_reading/writing also applies here.
| @@ -25,21 +25,20 @@ namespace rosbag2_storage | |||
|
|
|||
| StorageFactory::StorageFactory() | |||
| : impl_(new StorageFactoryImpl()) | |||
| { | |||
| } | |||
| {} | |||
|
|
|||
| StorageFactory::~StorageFactory() | |||
| { | |||
| delete impl_; | |||
There was a problem hiding this comment.
I would prefer having the pimpl using an unique_ptr instead of a raw pointer.
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
proposal for new storage interfaces