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.

Sign up for GitHub

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

Jump to bottom

Deduplicate code in SequentialCompressionReader #372

Merged
merged 2 commits into from
Apr 14, 2020
Merged

Deduplicate code in SequentialCompressionReader #372

merged 2 commits into from
Apr 14, 2020

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Apr 14, 2020 •
edited
Loading

Change the SequentialReader interfaces from private to protected. Then, delete all identical code from SequentialCompressionReader.

I'm about to add a new BaseReaderInterface function signature and don't want to duplicate it.

The existing test suite on these classes should cover the change.

Depends on #373
Starts replacing #371
In support of work towards #125

Signed-off-by: Emerson Knapp emerson.b.knapp@gmail.com

thomas-moulard
thomas-moulard approved these changes Apr 14, 2020
View reviewed changes
Copy link
Contributor

@thomas-moulard thomas-moulard left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - please double check whether some attributes in sequential_reader.hpp could be private?

@emersonknapp emersonknapp mentioned this pull request Apr 14, 2020
Closed
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Apr 14, 2020 •
edited
Loading

Sanity CI while I try to see it the Action CI failure is legit

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp emersonknapp mentioned this pull request Apr 14, 2020
Merged
piraka9011
piraka9011 approved these changes Apr 14, 2020
View reviewed changes
Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Pending Green CI

Karsten1987
Karsten1987 approved these changes Apr 14, 2020
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with green CI

rosbag2_compression/src/rosbag2_compression/sequential_compression_reader.cpp
{
storage_.reset();
}
{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= default?

rosbag2_cpp/include/rosbag2_cpp/readers/sequential_reader.hpp
Comment on lines +136 to +137
private:
std::shared_ptr<SerializationFormatConverterFactoryInterface> converter_factory_{};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from all the things which moved into the protected section, why not the converter_factory? I am not proposing otherwise, just asking for the rationale.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every other previously-private member is currently referenced directly in the SequentialCompressionReader subclass. We can probably clean it up a little so that more can be private, but my intention with this was to go for the minimum necessary set of visibility increase. Which happened to just be everything except this

@emersonknapp
Deduplicate code in SequentialCompressionReader
8a7ce11 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Make converter_factory_ private
f2fea42 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/sequentialreader-dedupe branch from 0affc95 to f2fea42 Compare April 14, 2020 17:55
@emersonknapp
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp
Copy link
Collaborator Author

All CI warnings are from upstream packages - the test failures are the persistent failures on Windows. The Action CI failure is also recurring on all PRs right now.

I'm going to merge this PR and then focus on getting the Action CI green before adding anything new.

@emersonknapp emersonknapp merged commit 23fa2e6 into master Apr 14, 2020
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/sequentialreader-dedupe branch April 14, 2020 19:29
@emersonknapp emersonknapp mentioned this pull request May 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomas-moulard thomas-moulard thomas-moulard approved these changes

@Karsten1987 Karsten1987 Karsten1987 approved these changes

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@piraka9011 piraka9011 piraka9011 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@emersonknapp @thomas-moulard @Karsten1987 @zmichaels11 @piraka9011