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

Add remove_from_all method to Corpus trait #2259

Merged
merged 5 commits into from
May 30, 2024
Merged

Conversation

tokatoka
Copy link
Member

No description provided.

@tokatoka
Copy link
Member Author

@R9295 Can you check this?

@R9295
Copy link
Contributor

R9295 commented May 29, 2024

Looks like the LibfuzzerCorpus & ArtifactCorpus also need remove_from_all implementations.

LGTM otherwise

@@ -115,6 +115,13 @@ where
Ok(testcase)
}

/// Removes an entry from the corpus, returning it if it was present; considers both enabled and disabled testcases.
fn remove_from_all(&mut self, idx: CorpusId) -> Result<Testcase<Self::Input>, Error> {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't remove just remove from wherever? Why does it make a difference if the testcase is disabled or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

every thing without all does not touch disabled entry

Copy link
Member

Choose a reason for hiding this comment

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

IMHO remove should remove, no matter what

Copy link
Member

Choose a reason for hiding this comment

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

Like, ask yourself: Why would you ever not use the remove_from_all method? How would you end up with a valid CorpusId that you don't want to remove just because it's disabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok i'll change

@tokatoka tokatoka merged commit e912216 into main May 30, 2024
98 of 99 checks passed
@tokatoka tokatoka deleted the add_remove_from_all branch May 30, 2024 09:53
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.

None yet

3 participants