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

clp-s: Ensure ArchiveWriterOption is fully initialized when splitting archives. #462

Merged
merged 3 commits into from
Jun 25, 2024

Conversation

gibber9809
Copy link
Contributor

Description

This PR fixes a minor bug where after splitting an archive the print-archive-stats option can be set to a random value.

Validation performed

  • Checked that after the archive splits the print-archive-stats option seems to be preserved

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

At a high level, wouldn't it make more sense to store archive_write_option as a member of JsonParser and then only update the id when splitting an archive?

components/core/src/clp_s/JsonParser.cpp Outdated Show resolved Hide resolved
@gibber9809 gibber9809 requested a review from kirkrodrigues June 25, 2024 21:35
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

For the PR title, how about:

clp-s: Ensure ArchiveWriterOption is fully initialized when splitting archives.

@gibber9809 gibber9809 changed the title clp-s: Fix a bug where print-archive-stats option is sometimes not initialized clp-s: Ensure ArchiveWriterOption is fully initialized when splitting archives. Jun 25, 2024
@gibber9809 gibber9809 merged commit 8a2c0a8 into y-scope:main Jun 25, 2024
11 checks passed
jackluo923 pushed a commit to jackluo923/clp that referenced this pull request Dec 4, 2024
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.

2 participants