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 TracerX-Del and TracerX-WP #955

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

ArpitaDutta
Copy link
Contributor

This pull request adds support for the TracerX tool with two different interpolation methods.

@PhilippWendler
Copy link
Member

It seems that TracerX-WP is the same tool as TracerX-Del except for the executable, is it (same URL etc.)? In this case, the tool-info module should not be duplicated, because duplicated code is always a maintenance burden. If you need a separate tool-info module, please use inheritance to avoid redundant code.

Maybe you even want to avoid this at all and simply make TracerX-WP be executable in the same as with tracerx?

@ArpitaDutta
Copy link
Contributor Author

May I request you accept this merge request as it is as the file modification using inheritance does not seem to be straightforward right now?

@PhilippWendler
Copy link
Member

To me it looks like this would be pretty straightforward actually, more than for other cases where we already have tool-info modules that use inheritance. Just let the class in tracerx-wp.py inherit from the one in tracerx.py and overwrite the two methods.

@ArpitaDutta
Copy link
Contributor Author

I am just worrying that the class name in tracerx-wp.py is as same as in tracerx.py.

If I change the class name in tracerx-wp.py to inherit properties from tracerx.py it may create some side effects in other steps of execution.

@PhilippWendler
Copy link
Member

No, there is no problem here. Lots of existing tool-info modules do exactly this. Have a look at graves.py and cpachecker.py, for example.

@ArpitaDutta
Copy link
Contributor Author

ArpitaDutta commented Nov 4, 2023

I have updated the PR as suggested.

@PhilippWendler PhilippWendler merged commit 035b06a into sosy-lab:main Nov 6, 2023
3 checks passed
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