-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 filter for bytecode that ECJ generates for String in switch #735
Conversation
a6a2d7c
to
84816ea
Compare
@@ -134,4 +136,8 @@ public void merge(final AbstractInsnNode i1, final AbstractInsnNode i2) { | |||
fail(); | |||
} | |||
|
|||
public void replace(AbstractInsnNode i, List<AbstractInsnNode> branches) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be removed by/after #734
84816ea
to
50229ff
Compare
@@ -371,6 +378,22 @@ public void visitEnd() { | |||
nodeToInstruction.get(r).merge(i); | |||
} | |||
} | |||
|
|||
// Replace: | |||
for (final Map.Entry<AbstractInsnNode, List<AbstractInsnNode>> r : replacements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this can be done during counting without need to modify instructions.
50229ff
to
de697fa
Compare
@@ -41,4 +43,6 @@ | |||
*/ | |||
void merge(AbstractInsnNode i1, AbstractInsnNode i2); | |||
|
|||
void replace(AbstractInsnNode i, List<AbstractInsnNode> branches); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what exactly this method does... can you please add some JavaDoc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof I was about to ask your advice about how to document it based on what its implementation does 😉
Something like
/**
* Marks instruction that should be considered as having branches into the
* given instructions during computation of coverage.
*
* @param i
* instruction to be replaced
* @param branches
* targets of branches
*/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so it does actually not replace the instruction but its branches. In this case I would rename the method and parameters to
replaceBranches(origin, targets)
Also maybe a second sentence can clarify the behavior: "The original target branches of the origin
instruction are ignored."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As like before this change term "branch" is fragile - we consider that each instruction has at least one branch and will be counted as covered when at least one branch is covered:
jacoco/org.jacoco.core/src/org/jacoco/core/internal/analysis/MethodAnalyzer.java
Lines 381 to 388 in d0e461a
final int total = i.getBranches(); | |
final int covered = i.getCoveredBranches(); | |
final ICounter instrCounter = covered == 0 ? CounterImpl.COUNTER_1_0 | |
: CounterImpl.COUNTER_0_1; | |
final ICounter branchCounter = total > 1 | |
? CounterImpl.getInstance(total - covered, covered) | |
: CounterImpl.COUNTER_0_0; | |
coverage.increment(instrCounter, branchCounter, i.getLine()); |
So in some sense this replaces instruction and not just its branches:
jacoco/org.jacoco.core/src/org/jacoco/core/internal/analysis/MethodAnalyzer.java
Lines 389 to 411 in cc43220
final int total; | |
final int covered; | |
final List<AbstractInsnNode> r = replacements.get(i.getNode()); | |
if (r != null) { | |
int cb = 0; | |
for (AbstractInsnNode b : r) { | |
if (nodeToInstruction.get(b).getCoveredBranches() > 0) { | |
cb++; | |
} | |
} | |
total = r.size(); | |
covered = cb; | |
} else { | |
total = i.getBranches(); | |
covered = i.getCoveredBranches(); | |
} | |
final ICounter instrCounter = covered == 0 ? CounterImpl.COUNTER_1_0 | |
: CounterImpl.COUNTER_0_1; | |
final ICounter branchCounter = total > 1 | |
? CounterImpl.getInstance(total - covered, covered) | |
: CounterImpl.COUNTER_0_0; | |
coverage.increment(instrCounter, branchCounter, i.getLine()); |
and while we don't have use-case and don't need this, even
output.replace(original, Collections.singletonList(replacement))
works.
Thus not sure about renaming of method.
Renaming of second parameter into targets
sounds good.
And for first one feel that original
sounds better than origin
.
I like the idea to add second statement 👍 with slight rewording
The original target branches of the original instruction will be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, at least in the Analyzer I used the term "branch" as a synonym for a edge in the control flow graph. But with the JavaDoc we can leave the method as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof I added
- Javadoc to
IFilterOutput
- tests to
MethodAnalyzerTest
- changelog entry
m.visitLabel(h2); | ||
|
||
m.visitVarInsn(Opcodes.ALOAD, 2); | ||
m.visitLdcInsn("\0a"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy-paste mistake - here and below should be "b"
instead of "\0a"
.
94eb2c6
to
e9643d5
Compare
* @param targets | ||
* new targets of branches | ||
*/ | ||
void replace(AbstractInsnNode original, List<AbstractInsnNode> targets); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Godin Sorry for coming back to this API again. I still think the API is confusing and can be improved:
- "instruction to be replaced": The instruction is not replaced, only the CFG edges to its successors is replaced by new edges.
- The targets have no order and must be unique. Should be a
Set
.
Here is my proposal:
/**
* Replaces the outgoing branches of a given instruction with new target
* target instructions.
*
* @param source
* instruction which branches should be replaced
* @param newTargets
* new targets of the branches
*/
void replaceBranches(AbstractInsnNode source,
Set<AbstractInsnNode> newTargets);
I know I introduced the term "branch" again. But this is what we conistently use in the JaCoCo code base and from my point of view it simplifies the description so that the second phrase is not needed at all any more.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The targets have no order and must be unique. Should be a Set.
Actually there is order, which is IMO well defined and very important property of/in MethodAnalyzer
, otherwise how merge
works? 😉 In particular default
of switch
has number 0
:
jacoco/org.jacoco.core/src/org/jacoco/core/internal/analysis/MethodAnalyzer.java
Lines 334 to 335 in 63d55ac
int branch = 0; | |
visitSwitchTarget(dflt, branch); |
And so maybe List
will be less confusing and more consistent even if doesn't play role in this API ?
The instruction is not replaced, only the CFG edges to its successors is replaced by new edges.
As we discussed earlier and as in implementation - we don't modify CFG, this affects only afterwards computation.
I know I introduced the term "branch" again.
No problem with term "branch" - it is already in Javadoc.
Right now I realized that with or without renaming of method on replaceBranches
from this interface alone not clear that replacement of branches changes coverage of instruction and not just branch coverage.
/** * Replaces the outgoing branches of a given instruction with new target
Other methods use term mark
(consistency?) and state that they affect stage of computations, i.e. not the original CFG and not the propagation of probes, what again as was discussed earlier is quite important in this case. So not sure that this is cleaner/better than current
/** * Marks instruction that should be considered as having branches into the * given instructions during computation of coverage.
Can imagine combination of both:
/**
* Marks instruction whose outgoing branches should be replaced during
* computation of coverage.
*
* @param source
* instruction which branches should be replaced
* @param newTargets
* new targets of branches
*/
void replaceBranches(AbstractInsnNode source,
In either case to me seems that important things are missing in this documentation:
merge
is performed beforeignore
andreplace
ignore
doesn't affectmerge
and targets inreplace
, but affects source inreplace
- targets in
replace
are affected bymerge
s replace
overridesmerge
s ofsource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof as we decided by phone:
- used my proposal of "combined version" for JavaDoc
- method and parameters were renamed
List
was replaced bySet
We'll think later about other improvements of documentation.
4cead6c
to
517f923
Compare
517f923
to
52e59df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this impressive work!
Just waiting for the green build now.
No description provided.