Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Add option to record metadata about decompressed archive chunks. #485
clp-s: Add option to record metadata about decompressed archive chunks. #485
Changes from 2 commits
b665de2
e3a899b
a9e320c
f6f9f10
b413080
a643c4a
f916270
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can you add a brief comment on why you use "xor"? like "either be both unspecified, or must be both specified"
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.
Sure, I'll add a comment
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 chance that we can use constant defined in https://github.com/y-scope/clp/blob/main/components/core/src/clp/clo/constants.hpp?
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.
I thought about it, and the way that would probably make sense if we did want to share this stuff between clo/clp-s is to just have a utility class that creates the required BSON objects and hides these field names. Right now we do take some classes from clp, but it's in a kind of nasty way where we have to manually add all of the required files to our build target. Really what we should do is have a build target that builds a library of utilities that all of our different binaries can use.
Since that's a bit of a bigger refactor I wanted to avoid including it in this PR. Plus the namespace styles used by clo are inconsistent with what we do here, and I want to avoid mixing styles.
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.
Sure