-
Notifications
You must be signed in to change notification settings - Fork 73
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
clp-s: Delete clp_s::FileReader #664
Comments
@gibber9809 While I was familiarizing with the structure of the codebase, I came across this issue. I thought it was a good way to get an in-dept overview of the various modules. I made a commit with my changes, although it is probably too big to be put directly into a pull request. That is why I am starting to write comments here. I followed this methodology:
I would like to specify that I did not replace all occurrences of |
Sorry @aestriplex, but changing instances of clp_s::ErrorCode to clp::ErrorCode is definitely not required to fix this issue. The current main users of clp::FileReader in clp_s already convert clp::ErrorCode to clp_s::ErrorCode when returning/handle clp::ErrorCode locally without propagating it in some way. Some of that translation between clp::ErrorCode and clp_s::ErrorCode could be better, but it's not an immediate concern. The only real remaining user of clp_s::FileReader is the open(clp_s::FileReader&...) usage in clp_s::ZstdDecompressor + associated code, which can be safely deleted since it is unused. Besides that all you should have to do is delete includes for FileReader.hpp which were missed in previous commits, and add includes for "ErrorCode.hpp" where we had been depending on it being included indirectly via FileReader.hpp. |
@gibber9809 Ok, thank you for your comment. I had misinterpreted the task. Tomorrow I'll focus on this
|
Request
There are not many uses of clp_s::FileReader left in our codebase, and clp-s is increasingly using readers from clp, so it would be beneficial to remove clp_s::FileReader as a step towards consolidating the reader classes for clp and clp_s.
Possible implementation
The text was updated successfully, but these errors were encountered: