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.

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

Deduplicate code in SequentialCompressionReader #372

Merged
merged 2 commits into from
Apr 14, 2020

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Apr 14, 2020

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

Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

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
Copy link
Collaborator Author

emersonknapp commented Apr 14, 2020

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

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

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

{
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?

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

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants