Skip to content

Commit e629bf0

Browse files
Godinmarchof
Godin
authored and
marchof
committed
Filter switch on String for which ECJ omits last goto (#741)
1 parent 68ab19f commit e629bf0

5 files changed

Lines changed: 82 additions & 44 deletions

File tree

‎org.jacoco.core.test.validation.java7/src/org/jacoco/core/test/validation/java7/targets/StringSwitchTarget.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,13 +101,27 @@ private static void lookupswitch(Object s) {
101101
}
102102
}
103103

104+
private static void default_is_first(Object s) {
105+
switch (String.valueOf(s)) { // assertFullyCovered(0, 2)
106+
default:
107+
nop("default");
108+
break;
109+
case "a":
110+
nop("case a");
111+
break;
112+
}
113+
}
114+
104115
public static void main(String[] args) {
105116
covered("");
106117
covered("a");
107118
covered("b");
108119
covered("\0a");
109120

110121
handwritten("a");
122+
123+
default_is_first("");
124+
default_is_first("a");
111125
}
112126

113127
}

‎org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/FilterTestBase.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,14 @@
1212
package org.jacoco.core.internal.analysis.filter;
1313

1414
import static org.junit.Assert.assertArrayEquals;
15+
import static org.junit.Assert.assertEquals;
1516
import static org.junit.Assert.fail;
1617

1718
import java.util.ArrayList;
19+
import java.util.Collections;
20+
import java.util.HashMap;
1821
import java.util.List;
22+
import java.util.Map;
1923
import java.util.Set;
2024

2125
import org.objectweb.asm.tree.AbstractInsnNode;
@@ -30,6 +34,8 @@ public abstract class FilterTestBase {
3034

3135
private final List<Range> ignoredRanges = new ArrayList<Range>();
3236

37+
private final Map<AbstractInsnNode, Set<AbstractInsnNode>> replacedBranches = new HashMap<AbstractInsnNode, Set<AbstractInsnNode>>();
38+
3339
protected final IFilterOutput output = new IFilterOutput() {
3440
public void ignore(final AbstractInsnNode fromInclusive,
3541
final AbstractInsnNode toInclusive) {
@@ -46,7 +52,7 @@ public void merge(final AbstractInsnNode i1,
4652

4753
public void replaceBranches(final AbstractInsnNode source,
4854
final Set<AbstractInsnNode> newTargets) {
49-
fail();
55+
replacedBranches.put(source, newTargets);
5056
}
5157
};
5258

@@ -59,6 +65,12 @@ final void assertMethodIgnored(final MethodNode m) {
5965
new Range(m.instructions.getFirst(), m.instructions.getLast()));
6066
}
6167

68+
final void assertReplacedBranches(final AbstractInsnNode source,
69+
final Set<AbstractInsnNode> newTargets) {
70+
assertEquals(Collections.singletonMap(source, newTargets),
71+
replacedBranches);
72+
}
73+
6274
static class Range {
6375
AbstractInsnNode fromInclusive;
6476
AbstractInsnNode toInclusive;

‎org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/StringSwitchEcjFilterTest.java‎

Lines changed: 51 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,6 @@
1111
*******************************************************************************/
1212
package org.jacoco.core.internal.analysis.filter;
1313

14-
import static org.junit.Assert.assertEquals;
15-
import static org.junit.Assert.assertNull;
16-
import static org.junit.Assert.fail;
17-
1814
import java.util.HashSet;
1915
import java.util.Set;
2016

@@ -28,39 +24,10 @@
2824
/**
2925
* Unit tests for {@link StringSwitchEcjFilter}.
3026
*/
31-
public class StringSwitchEcjFilterTest {
27+
public class StringSwitchEcjFilterTest extends FilterTestBase {
3228

3329
private final IFilter filter = new StringSwitchEcjFilter();
3430

35-
private final FilterContextMock context = new FilterContextMock();
36-
37-
private AbstractInsnNode fromInclusive;
38-
private AbstractInsnNode toInclusive;
39-
40-
private AbstractInsnNode source;
41-
private Set<AbstractInsnNode> newTargets;
42-
43-
private final IFilterOutput output = new IFilterOutput() {
44-
public void ignore(final AbstractInsnNode fromInclusive,
45-
final AbstractInsnNode toInclusive) {
46-
assertNull(StringSwitchEcjFilterTest.this.fromInclusive);
47-
StringSwitchEcjFilterTest.this.fromInclusive = fromInclusive;
48-
StringSwitchEcjFilterTest.this.toInclusive = toInclusive;
49-
}
50-
51-
public void merge(final AbstractInsnNode i1,
52-
final AbstractInsnNode i2) {
53-
fail();
54-
}
55-
56-
public void replaceBranches(final AbstractInsnNode source,
57-
final Set<AbstractInsnNode> newTargets) {
58-
assertNull(StringSwitchEcjFilterTest.this.source);
59-
StringSwitchEcjFilterTest.this.source = source;
60-
StringSwitchEcjFilterTest.this.newTargets = newTargets;
61-
}
62-
};
63-
6431
@Test
6532
public void should_filter() {
6633
final Set<AbstractInsnNode> expectedNewTargets = new HashSet<AbstractInsnNode>();
@@ -81,9 +48,9 @@ public void should_filter() {
8148
m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/String", "hashCode",
8249
"()I", false);
8350
m.visitTableSwitchInsn(97, 98, caseDefault, h1, h2);
51+
final AbstractInsnNode switchNode = m.instructions.getLast();
8452

8553
m.visitLabel(h1);
86-
final AbstractInsnNode expectedFromInclusive = m.instructions.getLast();
8754

8855
m.visitVarInsn(Opcodes.ALOAD, 2);
8956
m.visitLdcInsn("a");
@@ -115,9 +82,6 @@ public void should_filter() {
11582
m.visitJumpInsn(Opcodes.GOTO, caseDefault);
11683
final AbstractInsnNode expectedToInclusive = m.instructions.getLast();
11784

118-
m.visitLabel(caseDefault);
119-
m.visitInsn(Opcodes.RETURN);
120-
expectedNewTargets.add(m.instructions.getLast());
12185
m.visitLabel(case1);
12286
m.visitInsn(Opcodes.RETURN);
12387
expectedNewTargets.add(m.instructions.getLast());
@@ -127,13 +91,58 @@ public void should_filter() {
12791
m.visitLabel(case3);
12892
m.visitInsn(Opcodes.RETURN);
12993
expectedNewTargets.add(m.instructions.getLast());
94+
m.visitLabel(caseDefault);
95+
m.visitInsn(Opcodes.RETURN);
96+
expectedNewTargets.add(m.instructions.getLast());
97+
98+
filter.filter(m, context, output);
99+
100+
assertReplacedBranches(switchNode, expectedNewTargets);
101+
assertIgnored(new Range(switchNode.getNext(), expectedToInclusive));
102+
}
103+
104+
@Test
105+
public void should_filter_when_default_is_first() {
106+
final Set<AbstractInsnNode> expectedNewTargets = new HashSet<AbstractInsnNode>();
107+
108+
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0,
109+
"name", "()V", null, null);
110+
111+
final Label case1 = new Label();
112+
final Label caseDefault = new Label();
113+
final Label h1 = new Label();
114+
115+
m.visitVarInsn(Opcodes.ALOAD, 1);
116+
117+
m.visitVarInsn(Opcodes.ASTORE, 2);
118+
m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/String", "hashCode",
119+
"()I", false);
120+
m.visitLookupSwitchInsn(caseDefault, new int[] { 97 },
121+
new Label[] { h1 });
122+
final AbstractInsnNode switchNode = m.instructions.getLast();
123+
124+
m.visitLabel(h1);
125+
126+
m.visitVarInsn(Opcodes.ALOAD, 2);
127+
m.visitLdcInsn("a");
128+
m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/String", "equals",
129+
"(Ljava/lang/Object;)Z", false);
130+
// if equal "a", then goto its case
131+
m.visitJumpInsn(Opcodes.IFNE, case1);
132+
133+
final AbstractInsnNode expectedToInclusive = m.instructions.getLast();
134+
135+
m.visitLabel(caseDefault);
136+
m.visitInsn(Opcodes.RETURN);
137+
expectedNewTargets.add(m.instructions.getLast());
138+
m.visitLabel(case1);
139+
m.visitInsn(Opcodes.RETURN);
140+
expectedNewTargets.add(m.instructions.getLast());
130141

131142
filter.filter(m, context, output);
132143

133-
assertEquals(expectedFromInclusive.getPrevious(), source);
134-
assertEquals(expectedNewTargets, newTargets);
135-
assertEquals(expectedFromInclusive, fromInclusive);
136-
assertEquals(expectedToInclusive, toInclusive);
144+
assertReplacedBranches(switchNode, expectedNewTargets);
145+
assertIgnored(new Range(switchNode.getNext(), expectedToInclusive));
137146
}
138147

139148
}

‎org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/StringSwitchEcjFilter.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ public void match(final AbstractInsnNode start,
8585
// jump to default
8686
nextIs(Opcodes.GOTO);
8787
break;
88+
} else if (cursor.getNext() == defaultLabel) {
89+
break;
8890
}
8991
}
9092
}

‎org.jacoco.doc/docroot/doc/changes.html‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ <h3>New Features</h3>
3434
<li>Part of bytecode generated by ECJ for <code>switch</code> statements on
3535
<code>java.lang.String</code> values is filtered out during generation of
3636
report
37-
(GitHub <a href="https://github.com/jacoco/jacoco/issues/735">#735</a>).</li>
37+
(GitHub <a href="https://github.com/jacoco/jacoco/issues/735">#735</a>,
38+
<a href="https://github.com/jacoco/jacoco/issues/741">#741</a>).</li>
3839
<li>Methods added by the Kotlin compiler are filtered out during generation
3940
of report. Idea and implementation by Nikolay Krasko
4041
(GitHub <a href="https://github.com/jacoco/jacoco/issues/689">#689</a>).</li>

0 commit comments

Comments
 (0)