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 tool-info module for Rizzer #948

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Conversation

adamstafa
Copy link
Contributor

This pull request adds support for the tool Rizzer. It's based off of Fizzer merged in #921, hence the structure is very similar.

@adamstafa adamstafa changed the title Add Rizzer tool Add tool-info module for Rizzer Nov 1, 2023
@PhilippWendler
Copy link
Member

If the tool is based on Fizzer, does this mean that they will also share future development? I.e., if the output of Fizzer changes and the tool-info module needs to be updated, is it likely that the tool-info module of Rizzer will also need to be updated as well?

If yes, then inheritance should be used to avoid the duplicate code and the associated maintenance burden.

@adamstafa
Copy link
Contributor Author

Both tools currently have a separate branch in the repository and will diverge a bit in the near future. We plan to merge them at some point, but until then, it is easier to have an independent tool-info module for each tool.

@PhilippWendler
Copy link
Member

Do you plan in letting them diverge with respect to their inputs and outputs, i.e., those things that would reflect anything here in the tool-info module? Especially if you plan anyway on integrating them, I expect this to not be the case and thus the common code in the tool-info modules should be refactored out.

@adamstafa
Copy link
Contributor Author

adamstafa commented Nov 2, 2023

The output will likely change for both tools. Fizzer's output has been changing a lot in the past few months and Rizzer will probably output some information regarding potential crashes of Klee that is used. I'm not sure at this point what parts of the output will be shared between both tools, which makes factoring-out the common code a bit harder.

If you insist, I can create a common parent class for the tools, but I don't think it is worth it at this early stage of development.

(Sorry for the close and reopen, I missclicked)

@adamstafa adamstafa closed this Nov 2, 2023
@adamstafa adamstafa reopened this Nov 2, 2023
@PhilippWendler
Copy link
Member

First, note that once this is merged here and used, if you update your tool's output later we will expect to keep backwards compatibility of the tool-info module with old versions of the tool. This is in order to let people in the future with newer BenchExec replicate benchmarks with old tool versions. So consider this when making changes, maybe you can reduce the maintenance burden that comes with this.

Second, as long as it is unclear what will change, and given the fact that the current code needs to stay there anyway for backwards compatibility, it seems clear that having that much duplicated code is not good practice and will make maintenance more effort. Thus please refactor.

@adamstafa
Copy link
Contributor Author

I've changed the module so it inherits Fizzer's module. Is this alright?

@PhilippWendler PhilippWendler merged commit 27610c8 into sosy-lab:main Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants