fixed prematurely read issue of assignability analysis #190
Conversation
...lidate/src/test/java/org/opalj/fpcf/fixtures/xl/js/sandbox/JSAllocationWriteFieldFromJS.java
Outdated
Show resolved
Hide resolved
...cala/org/opalj/tac/fpcf/analyses/fieldassignability/AbstractFieldAssignabilityAnalysis.scala
Outdated
Show resolved
Hide resolved
|
|
||
| if (field.isPublic) | ||
| if (field.isPublic && !field.isFinal) |
There was a problem hiding this comment.
Wouldn't it be easier and more readable to just wrap the entire section into a separate if (!field.isFinal)?
| } else { | ||
| receiverVar.isDefined && receiverVar.get.definedBy != SelfReferenceParameter | ||
| } | ||
| if (method.isInitializer && method.classFile == field.classFile) { |
There was a problem hiding this comment.
I think this part of the code needs documentation to understand what all of the different conditions do
...main/scala/org/opalj/tac/fpcf/analyses/fieldassignability/L2FieldAssignabilityAnalysis.scala
Outdated
Show resolved
Hide resolved
...main/scala/org/opalj/tac/fpcf/analyses/fieldassignability/L2FieldAssignabilityAnalysis.scala
Outdated
Show resolved
Hide resolved
|
|
||
| C() { | ||
| super(); | ||
| //this.b(this); |
There was a problem hiding this comment.
Why is this commented out? If it is needed, reinstantiate it, if it is unnecessary, remove it.
There was a problem hiding this comment.
This was the example Marc gave me. I will remove the outcommented code as well as the function b()
| static int n = 5; | ||
|
|
||
| public static void main(String[] args) { | ||
| System.out.println("Value A.X before constructor:" + PrematurelyReadOfFinalField.n); |
There was a problem hiding this comment.
Are these printlns necessary?
There was a problem hiding this comment.
I will remove them.
…m/opalj/opal into fix/assignability-prematurelyRead
…assignments of array values
| // in different branches | ||
| if (!definedMethod.definedMethod.isConstructor) | ||
| return true | ||
| }; // Otherwise: field is written in multiple locations, thus must be assignable |
There was a problem hiding this comment.
Move semicolon to the return please
| if (index > 0 && stmts(index).isPutStatic) { | ||
| stmts(index).asPutStatic.value.asVar | ||
| } else if ( | ||
| index > 0 && stmts(index).isPutField && stmts(index).asPutField.value.asVar.value.isArrayValue.isYes |
There was a problem hiding this comment.
Please document this case, it is unclear why it is like this. In particular, why do we take the value from all PutStatic instructions, but only PutField instructions with array values? And why can't we rely on the receiverVar in all cases? Shouldn't it be same the asPut{Static,Field}.value.asVar anyway?
| if (index > 0 && stmts(index).isPutStatic) { | ||
| stmts(index).asPutStatic.value.asVar | ||
| } else if ( | ||
| index > 0 && stmts(index).isPutField && stmts(index).asPutField.value.asVar.value.isArrayValue.isYes |
There was a problem hiding this comment.
What if it is unknown whether the value is an array value? (Not sure if this can happen here, but still, one should think about it)
| } else { | ||
| receiverVar.isDefined && receiverVar.get.definedBy != SelfReferenceParameter | ||
| } | ||
| if (method.isInitializer && method.classFile == field.classFile) { |
No description provided.