Skip to content

StringSwitchJavacFilter fails to recognize switch that has just a few cases #730

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.

Sign up for GitHub

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

Jump to bottom
Merged
marchof merged 1 commit into from
Aug 13, 2018
Merged

StringSwitchJavacFilter fails to recognize switch that has just a few cases #730

marchof merged 1 commit into from
Aug 13, 2018

Conversation

@Godin
Copy link
Member

@Godin Godin commented Aug 13, 2018 •
edited
Loading

StringSwitchJavacFilter fails to recognize switch that has just a few cases

	private static void example(Object s) {
		switch (String.valueOf(s)) {
		case "a":
			nop("case a");
			break;
		case "b":
			nop("case b");
			break;
		}
	}

In this case javac generates lookupswitch, while filter expects tableswitch -

jacoco/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/StringSwitchJavacFilter.java

Line 94 in 90af085

nextIs(Opcodes.TABLESWITCH);

lookupswitch was noticed by @forax during my talk at Devoxx France and seems to be due to following algorithm that estimates space and time costs between lookupswitch and tableswitch - https://github.com/dmlloyd/openjdk/blob/84ba2b90d46ee4b06664d32594ef0cd2038373a6/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java#L1170

@Godin Godin self-assigned this Aug 13, 2018
@Godin
Copy link
Member Author

Godin commented Aug 13, 2018

@marchof could you please review?

Also maybe you can advice how to recording this in changelog - wording? bug fix or new feature?

@Godin Godin requested a review from marchof August 13, 2018 18:54
@Godin Godin added this to the 0.8.2 milestone Aug 13, 2018
@marchof marchof merged commit aa77868 into master Aug 13, 2018
@marchof marchof deleted the issue-730 branch August 13, 2018 20:23
@marchof
Copy link
Member

marchof commented Aug 13, 2018

@Godin Ups, sorry, I overlooked your question. For me is a bug fix, because we claimed that we can filter switch in string for javac. But probably no one ever noticed this corner case. Anyway, if we want to fix the latest commit here is my proposal:

Also filter part of bytecode generated for switch statements on java.lang.String values when only a few cases are present.

@Godin
Copy link
Member Author

Godin commented Aug 13, 2018

@marchof Changelog is also useful for us 😉 Fixed in f2c79fe

@Godin Godin added the type: bug 🐛 Something isn't working label Aug 13, 2018
@jacoco jacoco locked as resolved and limited conversation to collaborators Oct 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@marchof marchof marchof approved these changes

Assignees

@Godin Godin

Labels

component: core.filters language: Java type: bug 🐛 Something isn't working

Projects

Current work items
Archived in project
Filtering
Status: Done

Milestone

0.8.2

Development

Successfully merging this pull request may close these issues.

3 participants

@Godin @marchof