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 test setup for alias analysis #178

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

LukasKlenner
Copy link
Contributor

This PR adds a setup for testing alias analyses, including some (simple) test cases.

More (complex) test cases alongside analyses that pass them will be added in future PR's.
This is a continuation of #171

@errt
Copy link
Collaborator

errt commented Jan 9, 2024

Build doesn't work for several reasons, please fix

@LukasKlenner
Copy link
Contributor Author

building issues are fixed, but it fails because the AliasTests don't pass, despite them being annotated with @ignored. Is it not supported or is something else required to make the tests not being executed?

@LukasKlenner
Copy link
Contributor Author

PS: AliasEntity and AliasSourceElement need to be in the tac package (and not br) because they rely on the DefinitionSite class

@errt
Copy link
Collaborator

errt commented Jan 11, 2024

PS: AliasEntity and AliasSourceElement need to be in the tac package (and not br) because they rely on the DefinitionSite class

This may hint at potential to improve. In many cases, we map TAC use sites to bytecode program counters and values (search for persistentUVar), a similar mapping, maybe even excluding the values, might be applicable here.

@LukasKlenner
Copy link
Contributor Author

All previous issues have been addressed, but most of the classes have been completely reworked

*
* @see [[org.opalj.tac.fpcf.analyses.cg.persistentUVar]]
*/
case class PersistentUVar(valueInformation: ValueInformation, defSites: IntTrieSet)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be part of this file, but be in a generic place. You might want to take the code from here:
https://github.com/opalj/opal/pull/1/files#diff-62df04527a62216c2330e018931c309e5a1b88dac0e66e6a11e451a89dcf0517
and the following files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should i handle that the other PR is not merged yet? Just copy&paste the relevant code and add it to this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's probably the only practical solution for now. Unless that part of the code is in isolated commits (I didn't check that), then you could cherry-pick them.

Copy link
Contributor Author

@LukasKlenner LukasKlenner Jun 14, 2024

Choose a reason for hiding this comment

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

Cherry-picking this commit (c3b9715) should be sufficient. However, the class has been created in the tac package, therefore i can't use it as it is. I could move it to the br package but i don't know if this is intended and that should be done in the other PR as well to avoid duplicates after merging both.

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

2 participants