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 bytecode that javac generates for String in switch #596

Merged
merged 1 commit into from Sep 24, 2017

Conversation

Projects
2 participants
@Godin
Member

Godin commented Sep 21, 2017

I propose to do one more baby step in story of filtering - add filter for bytecode that javac generates for String in switch. And modify it for ECJ separately a bit later since it is a bit more tricky.

@Godin Godin added this to the 0.8.0 milestone Sep 21, 2017

@Godin Godin self-assigned this Sep 21, 2017

@Godin Godin added this to IN PROGRESS in Filtering Sep 21, 2017

@Godin Godin requested a review from marchof Sep 21, 2017

@marchof

Beside minor naming issues I think this filter is not specific enough: It filters every switch on String.hashCode(), e.g.

	switch (s.hashCode()) { // $line-falsePositive.switch$
	case 42:
		nop(); // $line-falsePositive.case42$
	case 123:
		nop(); // $line-falsePositive.case123$
	}

I don't think it is very likely that we find such cases in real code but I think the filter could be more specific.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Sep 23, 2017

Member

@Godin BTW, +1 for moving on with baby steps, thanks!

Member

marchof commented Sep 23, 2017

@Godin BTW, +1 for moving on with baby steps, thanks!

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Sep 23, 2017

Member

I think this filter is not specific enough: It filters every switch on String.hashCode()
I don't think it is very likely that we find such cases in real code but I think the filter could be more specific.

Well, was about to do push-back and say "if unlikely, then why complexify? This will make step bigger and harder than baby". But actually making it more specific wasn't that hard thanks to previously implemented AbstractMatcher and implementation with it is quite nice. So lets say that I was lazy 😉
Please re-review.

Member

Godin commented Sep 23, 2017

I think this filter is not specific enough: It filters every switch on String.hashCode()
I don't think it is very likely that we find such cases in real code but I think the filter could be more specific.

Well, was about to do push-back and say "if unlikely, then why complexify? This will make step bigger and harder than baby". But actually making it more specific wasn't that hard thanks to previously implemented AbstractMatcher and implementation with it is quite nice. So lets say that I was lazy 😉
Please re-review.

@marchof

marchof requested changes Sep 23, 2017 edited

Nice, thx!
Have two minor suggestions for the Matcher. Not mandatory though.

for (int i = 0; cursor != null && i < 4; i++) {
cursor = cursor.getPrevious();
}
if (cursor == null || cursor.getOpcode() != Opcodes.ICONST_M1) {

This comment has been minimized.

@marchof

marchof Sep 23, 2017

Member

Couldn't that be nextIs(Opcodes.ICONST_M1) followed by if (cursor == null) return;?

@marchof

marchof Sep 23, 2017

Member

Couldn't that be nextIs(Opcodes.ICONST_M1) followed by if (cursor == null) return;?

This comment has been minimized.

@Godin

Godin Sep 23, 2017

Member

@marchof Unfortunately no: nextIs first moves to next instruction and then checks, so can't be used to verify current instruction, and we can't place cursor before very first instruction of method.

@Godin

Godin Sep 23, 2017

Member

@marchof Unfortunately no: nextIs first moves to next instruction and then checks, so can't be used to verify current instruction, and we can't place cursor before very first instruction of method.

This comment has been minimized.

@Godin

Godin Sep 23, 2017

Member

@marchof this is kind of catch 22 - either we need to special case checking of beginning, or will have problem with end of methods, i.e. if we change semantic and cursor will be pointing to instruction for checking and moved after checking, then null can not be used to represent no-match as it is now, because next of last instruction is null. Maybe we can represent no-match by a special non-null value, but will be better to do this separately from this PR as it requires change of semantics of next methods and cursor.

@Godin

Godin Sep 23, 2017

Member

@marchof this is kind of catch 22 - either we need to special case checking of beginning, or will have problem with end of methods, i.e. if we change semantic and cursor will be pointing to instruction for checking and moved after checking, then null can not be used to represent no-match as it is now, because next of last instruction is null. Maybe we can represent no-match by a special non-null value, but will be better to do this separately from this PR as it requires change of semantics of next methods and cursor.

This comment has been minimized.

@Godin

Godin Sep 23, 2017

Member

@marchof another option is to have fake instruction at the beginning:

private void moveBack(int num) {
	for (int i = 0; i < num; i++) {
		if (cursor.getPrevious() == null) {
			cursor = new InsnNode(Opcodes.NOP) {
				final AbstractInsnNode next = cursor;
				@Override
				public AbstractInsnNode getNext() {
					return next;
				}
			};
		} else {
			cursor = cursor.getPrevious();
		}
	}
}

But I doubt that this is a good idea in general and definitely too much of complexity if done only here, compared to current handling of beginning.

@Godin

Godin Sep 23, 2017

Member

@marchof another option is to have fake instruction at the beginning:

private void moveBack(int num) {
	for (int i = 0; i < num; i++) {
		if (cursor.getPrevious() == null) {
			cursor = new InsnNode(Opcodes.NOP) {
				final AbstractInsnNode next = cursor;
				@Override
				public AbstractInsnNode getNext() {
					return next;
				}
			};
		} else {
			cursor = cursor.getPrevious();
		}
	}
}

But I doubt that this is a good idea in general and definitely too much of complexity if done only here, compared to current handling of beginning.

boolean match(final AbstractInsnNode start,
final AbstractInsnNode secondSwitchLabel) {
cursor = start;
for (int i = 0; cursor != null && i < 4; i++) {

This comment has been minimized.

@marchof

marchof Sep 23, 2017

Member

Would be nice to have a fluent API for this in Matcher

moveBack(4);

or something like this.

@marchof

marchof Sep 23, 2017

Member

Would be nice to have a fluent API for this in Matcher

moveBack(4);

or something like this.

This comment has been minimized.

@Godin

Godin Sep 23, 2017

Member

@marchof I would prefer to leave this as is and extract only if will be used somewhere else, otherwise IDE complains that value of parameter is always 4.

@Godin

Godin Sep 23, 2017

Member

@marchof I would prefer to leave this as is and extract only if will be used somewhere else, otherwise IDE complains that value of parameter is always 4.

@Godin Godin added this to IN PROGRESS in Current work items Sep 23, 2017

@marchof

OK, then good to me!

@marchof marchof merged commit 04f923c into master Sep 24, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@marchof marchof deleted the issue-596 branch Sep 24, 2017

@Godin Godin removed this from IN PROGRESS in Current work items Sep 24, 2017

@Godin Godin moved this from IN PROGRESS to DONE in Filtering Sep 24, 2017

@jacoco jacoco locked as resolved and limited conversation to collaborators Jan 11, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.