Skip to content
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

StringSwitchEcjFilter fails to recognize switch where default is first #741

Merged
merged 1 commit into from Aug 18, 2018

Conversation

Godin
Copy link
Member

@Godin Godin commented Aug 18, 2018

	private static void example(Object s) {
		switch (String.valueOf(s)) { // assertFullyCovered(0, 2)
			default:
				nop("default");
				break;
			case "a":
				nop("case a");
				break;
		}
	}

in this case ECJ omits last goto expected by filter -

if (cursor.getNext().getOpcode() == Opcodes.GOTO) {
// end of comparisons for same hashCode
// jump to default
nextIs(Opcodes.GOTO);

Looks like that this is due to optimization in algorithm that generates goto instructions - https://github.com/eclipse/eclipse.jdt.core/blob/97b95fa05a64615fc7296744b69bd741cc4af2c7/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/codegen/CodeStream.java#L3544

which is used by algorithm that generates code for switch on String - https://github.com/eclipse/eclipse.jdt.core/blob/97b95fa05a64615fc7296744b69bd741cc4af2c7/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java#L245-L269

@Godin Godin self-assigned this Aug 18, 2018
@Godin Godin added this to To Do in Filtering via automation Aug 18, 2018
@Godin Godin added this to Candidates in Current work items via automation Aug 18, 2018
@Godin Godin changed the title StringSwitchEcjFilter fails to recognize switch where default is first StringSwitchEcjFilter fails to recognize switch where default is first Aug 18, 2018
@Godin Godin added this to the 0.8.2 milestone Aug 18, 2018
Copy link
Member

@marchof marchof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, how did you even discover this? I've never seen someone putting default first.

@marchof marchof merged commit e629bf0 into master Aug 18, 2018
Filtering automation moved this from To Do to Done Aug 18, 2018
Current work items automation moved this from Candidates to Done Aug 18, 2018
@marchof marchof deleted the ecj_switch_string branch August 18, 2018 04:19
@Godin
Copy link
Member Author

Godin commented Aug 20, 2018

@marchof found this during check that #735 covers all cases from #496 , which mentions this case in comments 😉 don't remember how/why comment was originally added into #496 , however can admit that in C/C++ default as first case with early-return seems quite handy to me in some cases, especially in big switches

@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.
Projects
Filtering
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants