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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add filter for duplicates of finally blocks #604

Merged
merged 3 commits into from Dec 19, 2017

Conversation

Projects
2 participants
@Godin
Member

Godin commented Oct 4, 2017

Finally filter for finally, hopefully finally final version 馃槅

@Godin Godin added this to the 0.8.0 milestone Oct 4, 2017

@Godin Godin self-assigned this Oct 4, 2017

@Godin Godin added this to IN PROGRESS in Filtering Oct 4, 2017

@Godin Godin added this to IN PROGRESS in Current work items Oct 4, 2017

@Godin Godin requested a review from marchof Oct 4, 2017

@@ -44,7 +44,7 @@ public void test() {
// without filter next line has branches:
assertLine("test.close", ICounter.EMPTY);
assertLine("test.catch", ICounter.NOT_COVERED);
assertLine("test.finally", ICounter.PARTLY_COVERED);
assertLine("test.finally", ICounter.FULLY_COVERED);

This comment has been minimized.

@marchof

marchof Oct 5, 2017

Member

@Godin Just out of curiosity: The new FinallyFilter now also applies to tryWithResouces?

@marchof

marchof Oct 5, 2017

Member

@Godin Just out of curiosity: The new FinallyFilter now also applies to tryWithResouces?

This comment has been minimized.

@Godin

Godin Oct 5, 2017

Member

@marchof javac translates try-with-resources into try-catch-finally, so for it for sure applies. However most likely not applies for bytecode that ecj generates for closing of resources in try-with-resources. But applies for try-with-resources with finally for both ecj and javac as this test shows. Can add more precise tests about this, but definitely would prefer to do this in a separate pull request.

@Godin

Godin Oct 5, 2017

Member

@marchof javac translates try-with-resources into try-catch-finally, so for it for sure applies. However most likely not applies for bytecode that ecj generates for closing of resources in try-with-resources. But applies for try-with-resources with finally for both ecj and javac as this test shows. Can add more precise tests about this, but definitely would prefer to do this in a separate pull request.

This comment has been minimized.

@marchof

marchof Oct 5, 2017

Member

@Godin Ok, makes sense. I was wondering whether our validadtion tests for filters should only use the filter under test.

@marchof

marchof Oct 5, 2017

Member

@Godin Ok, makes sense. I was wondering whether our validadtion tests for filters should only use the filter under test.

This comment has been minimized.

@Godin

Godin Oct 5, 2017

Member

@marchof we already discussed this: for start it is very good to see combined behavior - this way with low investment we observe what end-user will observe and we observe that filters correctly cooperate and do not interfere with each other in a bad way. However I agree that in long run ability to execute filters separately will be also valuable.

@Godin

Godin Oct 5, 2017

Member

@marchof we already discussed this: for start it is very good to see combined behavior - this way with low investment we observe what end-user will observe and we observe that filters correctly cooperate and do not interfere with each other in a bad way. However I agree that in long run ability to execute filters separately will be also valuable.

This comment has been minimized.

@marchof

marchof Oct 5, 2017

Member

@Godin Sure. For I don't want to change test strategy.

@marchof

marchof Oct 5, 2017

Member

@Godin Sure. For I don't want to change test strategy.

} while (!(Opcodes.ALOAD == i.getOpcode()
&& var == ((VarInsnNode) i).var));
i = next(i);
if (Opcodes.ATHROW != i.getOpcode()) {

This comment has been minimized.

@marchof

marchof Oct 5, 2017

Member

@Godin Potential NPE? i could be null here -- but only for invalid class files. So probably not an issue.

@marchof

marchof Oct 5, 2017

Member

@Godin Potential NPE? i could be null here -- but only for invalid class files. So probably not an issue.

try {
nop();
} finally { // $line-alwaysCompletesAbruptly.0$
return; // $line-alwaysCompletesAbruptly.1$

This comment has been minimized.

@marchof

marchof Oct 6, 2017

Member

@Godin Here I get a warning in Eclipse, maybe annotate method with @SuppressWarnings("finally")?

@marchof

marchof Oct 6, 2017

Member

@Godin Here I get a warning in Eclipse, maybe annotate method with @SuppressWarnings("finally")?

This comment has been minimized.

@Godin

Godin Oct 6, 2017

Member

@marchof added this suppression, however warnings and suppressions are incredibly inconsistent in IDEs, e.g. even after addition I see warnings in this target file and in some other target files in IntelliJ. Ideally, I think, that our main code should not contain warnings and suppressions, but should be possible to write whatever in targets without bothering with warnings/suppressions.

@Godin

Godin Oct 6, 2017

Member

@marchof added this suppression, however warnings and suppressions are incredibly inconsistent in IDEs, e.g. even after addition I see warnings in this target file and in some other target files in IntelliJ. Ideally, I think, that our main code should not contain warnings and suppressions, but should be possible to write whatever in targets without bothering with warnings/suppressions.

This comment has been minimized.

@marchof

marchof Oct 6, 2017

Member

@Godin We could simply add @SuppressWarnings("all") to our validation targets. Should work for all IDEs.

@marchof

marchof Oct 6, 2017

Member

@Godin We could simply add @SuppressWarnings("all") to our validation targets. Should work for all IDEs.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Oct 6, 2017

Member

@Godin Looks like the latest update broke some tests.

Member

marchof commented Oct 6, 2017

@Godin Looks like the latest update broke some tests.

* </pre>
*/
@Test
public void should_analyze_control_flow() {

This comment has been minimized.

@marchof

marchof Oct 6, 2017

Member

@Godin While I'm trying to wrap my mind around the filter implementation I was wondering why we need to loop over all tryCatchBlocks with the same handler. This seems to be the only test case which triggers this. Therefore I wonder whether it might make sense to add this scenario also to the validation test.

@marchof

marchof Oct 6, 2017

Member

@Godin While I'm trying to wrap my mind around the filter implementation I was wondering why we need to loop over all tryCatchBlocks with the same handler. This seems to be the only test case which triggers this. Therefore I wonder whether it might make sense to add this scenario also to the validation test.

This comment has been minimized.

@Godin

Godin Oct 6, 2017

Member

@marchof done

@Godin

Godin Oct 6, 2017

Member

@marchof done

This comment has been minimized.

@marchof

marchof Oct 6, 2017

Member

@Godin When I remove the two loops like this:

	// for (final TryCatchBlockNode t : tryCatchBlocks) {
	{
		final TryCatchBlockNode t = finallyBlock;

the validation tests are still fully green (ECJ) and a different test fails for javac:

emptyCatch(org.jacoco.core.test.filter.FinallyTest): Status in line 79:	nop("finally"); // $line-emptyCatch.1$ expected:<[FUL]LY_COVERED> but was:<[PART]LY_COVERED>

But new test twoRegions() stays green. What am I missing here?

@marchof

marchof Oct 6, 2017

Member

@Godin When I remove the two loops like this:

	// for (final TryCatchBlockNode t : tryCatchBlocks) {
	{
		final TryCatchBlockNode t = finallyBlock;

the validation tests are still fully green (ECJ) and a different test fails for javac:

emptyCatch(org.jacoco.core.test.filter.FinallyTest): Status in line 79:	nop("finally"); // $line-emptyCatch.1$ expected:<[FUL]LY_COVERED> but was:<[PART]LY_COVERED>

But new test twoRegions() stays green. What am I missing here?

This comment has been minimized.

@Godin

Godin Oct 6, 2017

Member

@marchof after this modification of both loops

  • emptyCatch fails because condition of if statement in second loop always false
  • unit test for the case of twoRegions shows that merged too much, but validation test has assertion only for final, for same reason ecj stays green

Missing assertions added.

Also this test demonstrates https://bugs.openjdk.java.net/browse/JDK-7008643

@Godin

Godin Oct 6, 2017

Member

@marchof after this modification of both loops

  • emptyCatch fails because condition of if statement in second loop always false
  • unit test for the case of twoRegions shows that merged too much, but validation test has assertion only for final, for same reason ecj stays green

Missing assertions added.

Also this test demonstrates https://bugs.openjdk.java.net/browse/JDK-7008643

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Oct 6, 2017

Member

@marchof

Looks like the latest update broke some tests.

Ouch, forgot to update test that studies placement of GOTOs. Fixed.

Member

Godin commented Oct 6, 2017

@marchof

Looks like the latest update broke some tests.

Ouch, forgot to update test that studies placement of GOTOs. Fixed.

@marchof

marchof approved these changes Oct 7, 2017

@Godin Loods good to me know from a formal point of view! However I had a very hard time to understand the implementation.

Maybe the class comment can contain the description of the basic structures created by finally and our merge/ignore merge strategy.

Also maybe some inline comments, like:

  • why looping over all handlers with same target?
  • what exactly does the large while/switch block?

Godin added some commits Dec 12, 2017

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 16, 2017

Member

@marchof description and comments added, could you please re-review? If all fine, I'll squash and merge.

Member

Godin commented Dec 16, 2017

@marchof description and comments added, could you please re-review? If all fine, I'll squash and merge.

@marchof

Thanks @Godin for the great explanation!

@Godin Godin merged commit ff00194 into master Dec 19, 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

@Godin Godin deleted the issue-604 branch Dec 19, 2017

@Godin Godin moved this from IN PROGRESS to DONE in Filtering Dec 20, 2017

@Godin Godin removed this from IN PROGRESS in Current work items Dec 20, 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.