IDE Basics #240
IDE Basics #240
Conversation
Rkoer
commented
Nov 15, 2024
Rkoer
commented
- IDE infrastructure
- Basic IDE solver
- Example implementation of linear constant propagation
- Interaction capabilities
- Example implementation of a linear constant propagation on fields
…uch but no callees are found)
…ties when generating edge functions
Currently, there will only be more than one entry/statement if there are multiple return sites. In the future, there will be a possibility to query for a single statement, so the property store can be used to realize this distinction.
…/inner functions)
| /** | ||
| * The name of the variable the array is stored in | ||
| */ | ||
| String variable(); |
There was a problem hiding this comment.
Using (TAC) variable names could be fragile and is hard to understand by reading the source code. Maybe it would be better to identify variables by the line they are declared in.
There was a problem hiding this comment.
I guess we could add this, but there are tests (not the ones for LCPoF but esp. the ones for LCP) are more detailed and also check values that are not explicitly declared in the code.
There was a problem hiding this comment.
Can we identify them by program counter or line number then? Program counters are at least guaranteed to be stable since we use a fixed compiler. Line numbers are a little more fragile as one might make non-semantic changes to the source files, but they are easier to understand from the source code.
| @@ -0,0 +1,45 @@ | |||
| /* BSD 2-Clause License - see OPAL/LICENSE for details. */ | |||
| package org.opalj.fpcf.fixtures.lcp_on_fields; | |||
There was a problem hiding this comment.
Could it be sensible to move these into a subpackage of the fixtures.linear_constant_propagation package?
There was a problem hiding this comment.
That sounds logical. However, at the moment there are two separate test classes, one for LCP and one for LCPoF. When moving the fixtures inside the linear_constant_propagation package then the tests for LCP do pick up the fixtures for LCPoF too. I guess we would need to specify each test fixture for LCP manually instead of providing the package. Is there an alternative solution to this?
There was a problem hiding this comment.
How about one fixture directory with two subdirectories, for LCP and LCPoF?
| * | ||
| * @author Robin Körkemeier | ||
| */ | ||
| class IDEPropertiesTest extends PropertiesTest { |
There was a problem hiding this comment.
This looks like it should be a trait / abstract class
| * | ||
| * @author Robin Körkemeier | ||
| */ | ||
| object LCPOnFieldsAnalysisScheduler |
There was a problem hiding this comment.
I don't think you need this as a separate object, it should be possible to just put new LCPOnFieldsAnalysisScheduler() with JavaIDEAnalysisSchedulerBase.RTACallGraph at its single use. If it is needed, I caution agains naming it the same as its parent class, to avoid confusion.
| * | ||
| * @author Robin Körkemeier | ||
| */ | ||
| abstract class AbstractRepeatablePropertyMatcher extends AbstractPropertyMatcher { |
There was a problem hiding this comment.
This seems like a good idea. Can you apply it to the DirectCallMatcher as well (maybe in a separate PR)?
| set.add(c) | ||
| } | ||
|
|
||
| def areDependeesEmpty: Boolean = { |
There was a problem hiding this comment.
It would be more conventional to have a hasDependees method with inverted semantics
| /** | ||
| * @return whether the phase is finished or has to be continued once the dependees are resolved | ||
| */ | ||
| private def performPhase1()(implicit s: State): Boolean = { |
There was a problem hiding this comment.
Phase1/Phase2 are very non-descriptive names. Try to come up with better ones and at the very least, document these methods properly to make the code understandable.
| ) | ||
| } | ||
|
|
||
| logDebug("finished creation of properties and results") |
There was a problem hiding this comment.
Consider whether these log messages are really necessary
| } | ||
| } | ||
|
|
||
| private def processCallFlow(path: Path, f: JumpFunction, qs: collection.Set[Callable])( |
There was a problem hiding this comment.
This code is poorly documented and uses many nondescriptives names. What are things like sp, n, rs, qs, d5, d5s, d3, sqs, etc.
| * | ||
| * @author Robin Körkemeier | ||
| */ | ||
| class IDEAnalysis[Fact <: IDEFact, Value <: IDEValue, Statement, Callable <: Entity]( |
There was a problem hiding this comment.
There are many complex methods in this class that should be properly documented, at least with a ScalaDoc on what they do.
