- Notifications
You must be signed in to change notification settings - Fork 1.2k
IFilterOutput.replaceBranches
should allow the use of individual branches of instructions #1813
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
Conversation
e140758
to cafdd71
Compare daf9099
to 336284f
Compare method.visitLdcInsn(""); | ||
// probe[3] | ||
method.visitLabel(l2); | ||
// probe[4] | ||
method.visitInsn(Opcodes.ARETURN); |
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 a side note:
Every time I touch MethodAnalyzerTest
I struggle to recall/realize how/where probes are inserted. Maybe it becomes easier after every touch, but still. That's why I started to add comments in the form // probe[id]
- they at least help to recall, but unfortunately not all tests have them, and unfortunately they do not appear magically for new tests 😅
IMO it would be nice to have a way to automatically show the places of probes or even better - assert them. Moreover MethodAnalyzerTest
is already indirectly testing LabelFlowAnalyzer
and MethodProbesAdapter
and IMO contains more interesting (and far more) scenarios than their tests.
Set<AbstractInsnNode> newTargets); | ||
| ||
/** | ||
* {@link #instruction} and index of its {@link #branch}. |
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.
It will be nice to add a comment about branch indexes for JumpInsnNode
, TableSwitchInsnNode
, LookupSwitchInsnNode
and others.
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.
Done in 348bfc0
Instruction i1 = new Instruction(1); | ||
Instruction i2 = new Instruction(2); | ||
Instruction i3 = new Instruction(3); |
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.
At least this violates Instruction
contract - see comment about use of Math.max
https://github.com/jacoco/jacoco/pull/1813/files#r1883065587
final HashMap<AbstractInsnNode, Collection<InstructionBranch>> replacements = new HashMap<AbstractInsnNode, Collection<InstructionBranch>>(); | ||
for (final AbstractInsnNode target : newTargets) { | ||
final ArrayList<InstructionBranch> list = new ArrayList<InstructionBranch>(); | ||
final int branches = Math.max( |
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 a side note:
Math.max
is used here because already existing unit tests
InstructionTeest.replaceBranches_should_calculate_coverage_on_new_branches
ClassAnalyzerTest.testCalculateFragments
ClassAnalyzerTest.should_add_non_empty_methods
violate contract of Instruction
that states that after CFG construction each instruction has at least one branch.
e9ceb5f
to 22958a9
Compare 22958a9
to 48641b1
Compare private void applyReplacements() { | ||
for (final Entry<AbstractInsnNode, Set<AbstractInsnNode>> entry : replacements | ||
for (final Entry<AbstractInsnNode, Iterable<Collection<InstructionBranch>>> entry : replacements | ||
.entrySet()) { | ||
final Set<AbstractInsnNode> replacements = entry.getValue(); | ||
final List<Instruction> newBranches = new ArrayList<Instruction>( | ||
replacements.size()); | ||
for (final AbstractInsnNode b : replacements) { | ||
newBranches.add(instructions.get(b)); | ||
final Iterable<Collection<InstructionBranch>> targets = entry | ||
.getValue(); | ||
int i = 0; | ||
for (final Collection<InstructionBranch> list : targets) { | ||
i += list.size(); | ||
} | ||
final int[] branches = new int[i]; | ||
final Instruction[] fromInstructions = new Instruction[i]; | ||
final int[] fromBranches = new int[i]; | ||
| ||
i = 0; | ||
int b = 0; | ||
for (final Collection<InstructionBranch> list : targets) { | ||
for (final InstructionBranch ib : list) { | ||
branches[i] = b; | ||
fromInstructions[i] = instructions.get(ib.instruction); | ||
fromBranches[i] = ib.branch; | ||
i++; | ||
} | ||
b++; | ||
} | ||
| ||
final AbstractInsnNode node = entry.getKey(); | ||
instructions.put(node, | ||
instructions.get(node).replaceBranches(newBranches)); | ||
instructions.put(node, instructions.get(node) | ||
.replaceBranches(branches, fromInstructions, fromBranches)); | ||
} | ||
} |
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 already did not like that the old code was doing allocations merely to pass the data between different interfaces, but the code was small enough, and now it become bigger 😞
IMO allocations can be avoided and code can be simplified by introduction of "mapper" interface similar to java.util.function.Function<AbstractInstructionNode, Instruction>
mapper = (AbstractInsnNode node) -> instructions.get(node); mapper.apply(node).replaceBranches(mapper, 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.
Done in 12778c3
52558c5
to 491c19f
Compare @marchof @leveretka could you please review? |
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.
Very impressive work that will unblock many Kotlin features. Thanks for working on that.
So far the core change looks very straightforward (thanks for splitting it into commits was much easier to review).
I will probably need a second look on the updated filters. But so far the change looks nice.
I would probably recommend splitting the PR into several commits. In the first you add a new API for
the replaceBranches
, and in the other ones you migrate filters. You already have it in several commits, maybe we can also merge it separately (Just an idea)
I still would like to hear @marchof opinion, as you change some internals.
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.
Since I don't see any concerns mentioned here, I think we can merge it
org.jacoco.core.test/src/org/jacoco/core/internal/analysis/InstructionTest.java Outdated Show resolved Hide resolved
org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/IFilterOutput.java Outdated Show resolved Hide resolved
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 have to admit that I wasn't able to wrap my mind around the functionality implemented here. So I left only minor comments regarding the code structure. @Godin please feel free to just ignore them if you don't agree.
@marchof I'm wondering if you read descriptions of #1772 and #1773 ? 😉 IMO they contain description/motivation for the main change here in * Creates a copy of this instruction where all outgoing branches are - * replaced with the given instructions. The coverage status of the new - * instruction is derived from the status of the given instructions. + * replaced. The coverage statuses of the branches of the new instruction + * are derived from the statuses of the given branches of the given + * instructions. |
org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/Replacements.java Outdated Show resolved Hide resolved
org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/FilterTestBase.java Outdated Show resolved Hide resolved
org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/Replacements.java Outdated Show resolved Hide resolved
Co-authored-by: Marc R. Hoffmann <hoffmann@mountainminds.com>
This is required for the resolution of
default
clause ofswitch
by String when compiled by ECJ #1772else
clause ofwhen
by String in Kotlin #1773(see explanations in the tickets)
Fixes #1772
Fixes #1773
Also current implementation does not preserve branch indexes in bytecode traversal order - see
jacoco/org.jacoco.core/src/org/jacoco/core/internal/analysis/Instruction.java
Lines 176 to 177 in e6f29c9
ie for example case when only first replacement target is executed becomes indistinguishable from case when only last replacement target is executed - in both cases resulting instruction will have branch
0
as executed, whereas indexes are important forI believe that this change is sufficient to cover this need too.