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

Add filter for Kotlin when-expressions with String #737

Merged
merged 2 commits into from
Aug 18, 2018

Conversation

Godin
Copy link
Member

@Godin Godin commented Aug 15, 2018

No description provided.

@Godin Godin added this to the 0.8.2 milestone Aug 15, 2018
@Godin Godin self-assigned this Aug 15, 2018
@Godin Godin added this to To Do in Filtering via automation Aug 15, 2018
@Godin Godin requested a review from marchof August 15, 2018 20:09
@Godin Godin added this to Review in Current work items via automation Aug 15, 2018
@Godin Godin moved this from To Do to In Progress in Filtering Aug 15, 2018
@Godin Godin force-pushed the kotlin_when_string branch 2 times, most recently from d1fe9d2 to e26c7e4 Compare August 15, 2018 20:11
@marchof
Copy link
Member

marchof commented Aug 17, 2018

@Godin Would prefer to review this after #735 is merged to master.

@Godin Godin moved this from Review to Implementation in Current work items Aug 17, 2018
@Godin Godin force-pushed the kotlin_when_string branch 2 times, most recently from 842bcd2 to cf5760d Compare August 17, 2018 20:32
@Godin
Copy link
Member Author

Godin commented Aug 17, 2018

@marchof rebased

Currently diff between StringSwitchEcjFilter and KotlinWhenStringFilter is

26,27c26,27
<  * Filters code that is generated by ECJ for a <code>switch</code> statement
<  * with a <code>String</code>.
---
>  * Filters bytecode that Kotlin compiler generates for <code>when</code>
>  * expressions with a <code>String</code>.
29c29
< public final class StringSwitchEcjFilter implements IFilter {
---
> public final class KotlinWhenStringFilter implements IFilter {
46a47
> 			nextIsVar(Opcodes.ALOAD, "s");
73a75,77
> 					// jump to next comparison or default case
> 					nextIs(Opcodes.IFEQ);
> 					final JumpInsnNode jump = (JumpInsnNode) cursor;
75c79
< 					nextIs(Opcodes.IFNE);
---
> 					nextIs(Opcodes.GOTO);
83c87
< 					if (cursor.getNext().getOpcode() == Opcodes.GOTO) {
---
> 					if (jump.label == defaultLabel) {
85,86d88
< 						// jump to default
< 						nextIs(Opcodes.GOTO);

😆 but I'd like to propose and would really prefer to work on refactoring separately.

@marchof marchof moved this from Implementation to Review in Current work items Aug 17, 2018
}
}

private static AbstractInsnNode instructionAfterLabel(
Copy link
Member

Choose a reason for hiding this comment

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

Idea: Make a variant of skipNonOpcodes in base class public:

private static AbstractInsnNode skipNonOpcodes(AbstractInsnNode start)

@marchof
Copy link
Member

marchof commented Aug 17, 2018

@Godin Do you want me to merge it as is or do you like to pick-up the skipNonOpcodes idea before?

@Godin
Copy link
Member Author

Godin commented Aug 17, 2018

@marchof A quick random thought: instructionAfterLabel(LabelNode) compared to skipNonOpcodes(AbstractInsnNode) is safer since accepts only LabelNode. So let's go without skipNonOpcodes - will think about this at a time of thinking about similarity between StringSwitchEcjFilter and KotlinWhenStringFilter.

@marchof
Copy link
Member

marchof commented Aug 17, 2018

👍 Let's wait for Travis.

@marchof marchof merged commit 32073ea into jacoco:master Aug 18, 2018
Filtering automation moved this from In Progress to Done Aug 18, 2018
Current work items automation moved this from Review to Done Aug 18, 2018
@Godin Godin deleted the kotlin_when_string branch August 18, 2018 18:07
@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