@marchof some thoughts about action plan to divide and conquer:

Ability to ignore instructions is not that hard to implement as shown here. And this is already valuable enhancement, because opens door for introduction of unconditional (i.e. non-configurable since non-questionable) filters:

• switch that javac generates for string in switch statement as shown here
• catch block for synchronized statement
• private empty constructor

The last two can be implemented as a next baby step after this PR.

Also opens door at least for experiments with filters that raise some questions:

• assert statements
• branch for logical complement operator

Filtering of bytecode that ECJ generates for string in switch statement has some tricky machinery as shown here. So maybe should be postponed, or could be introduced now and then refactored during further steps.

Then seems that filtering of finally is prerequisite for try-with-resources (even could be that this is the only necessary thing). Also seems that this requires merging of paths and not just ignoring of instructions, what seems tricky, but possible. And so this will lead to a necessary reorganization of analysis.

Implementation of the last two - are the final step for non-configurable/non-questionable filters IMO. But already will be a very good improvement, especially Java 7 try-with-resources. After which configurable ones (such as filtering based on comments in source code) can be taken into consideration.

After reading JLS and experimenting more with Java 7 try-with-resources, maybe exclusion of instructions will be enough, which will be good news, need to do more tests...

@marchof and... here is filter for try-with-resources - #500 😉

@Godin First of all: Thank your for pushing this forward! It's great to see that the implementation actually nicely fits on top of our existing analyzer infrastructure. Also the filtering interface is really simple, so we can optimize/rework both sides later if we want to.

Some remarks though:

• We should have a filter API, something like the static method you already defined. So we can implement and unit test each filter separately:

  public interface IFilter {
    String getId(); // see below
    void filter(final MethodNode methodNode, final IOutput output);
  }
  

• Maybe we move the filter implementations in a new internal package org.jacoco.core.internal.analysis.filter?

• We follow the naming conventions from the wiki for our filters: In this case JavacStringswitchFilter

• The same for the filter validation tests: org.jacoco.core.test.validation.filter

Filter activation:

I would like to add this very quickly to our master. But I'm not sure whether it should be activated by default. Therefore I propose to add a new method to the Analyzer:

void activateFilters(Set<String> filterIds);

We have a registry of all implemented filters. Each filter has an id like the ones in our wiki. But only the activated filters will be considered while analysis. Maven and Ant reporting goals will get a new property for filter ids.

So we can aggressively add new filters and ask people to test on real projects. Once we're confident with functionality and performance we can enable them as default.

Another comment about filter implementation: Navigating the ASM tree leads to awkward code. When I did my first experiments I implemented a simple matcher to describe bytecode sequences:

https://github.com/marchof/jacoco-playground-filters/tree/master/org.jacoco.playground.filter/src/main/java/org/jacoco/playground/filter

Something like this may improve readability of the filters, but we can add this later of course.

@Godin Regarding IFilter proposed by me: we might add another API to filter whole methods, so we can move the code from ClassAnalyzer.isMethodFiltered()to IFilterimplementation. But as said before, not required for this PR from my POV.

@marchof

We follow the naming conventions from the wiki for our filters: In this case JavacStringswitchFilter

Javac prefix is a bit misleading given presence of a code dedicated for ECJ.

Also I don't see strong connection with a wiki page here. After start of work on implementation test cases in implementation quite quickly uncover things that are not in wiki, making it outdated. So actually I expect that particular wiki pages will eventually be transformed into code/tests/javadocs/documentation, at least in respect to covered cases.

Thus what about simple StringSwitchFilter, since as of today we are not really targeting other JVM languages and there are no other Java compilers except javac and ECJ ?

Navigating the ASM tree leads to awkward code
When I did my first experiments I implemented a simple matcher to describe bytecode sequences

That's true, however attempt to design declarative matchers before obtaining knowledge of what is really needed for an implementation of particular filter IMO actually increases complexity and error-prone. For example I rewrote completely #500 already 2 times and not excluded that will do one more time. Also there are some imperative logic that is quite hard to express in a declarative way. So let's postpone this till the actual concrete moment of the need to improve readability.

I would like to add this very quickly to our master. But I'm not sure whether it should be activated by default.

What are the reasons of uncertainty?

I decided to implement filter for String in switch statement (this PR), try-with-resources statement (#500) and synchronized statement (#501) exactly because they are IMO unquestionable in terms of enablement - can't imagine that someone will ever want to disable them. Configurable filters (like the one based on comments in code) is completely different story.

Do you have fears about filter for synchronized statement (#501) ?

Maven and Ant reporting goals will get a new property for filter ids.
So we can aggressively add new filters and ask people to test on real projects.
Once we're confident with functionality and performance we can enable them as default.

I don't like addition of such configuration option. IMO more time will be wasted on explanations about how to use it and maintenance of it, rather than on valuable feedback. Also this will increase amount of information required to investigate/reproduce problems with analysis.

In terms of feedback IMO there are far better and simpler options. For example we can ask people to test snapshot version with enabled by default filters. Or even release a version - anyway there is always a possibility to use previous version.

And if there are concerns about performance, then I'd better test/work explicitly on this rather than on introduction and explanations of configuration options.

@Godin Ok, I can agree on your points! ;-)

What about the IFilter abstraction and separate package? Is that ok for you?

Regarding performance we can do a simple Benchmark by e.g. analyzing rt.jar and see the extra cost. Even if we don't make the filtering configurable we need a simple way to do comparisons in future.

What about the IFilter abstraction and separate package? Is that ok for you?

I updated #501 , so that it introduces IFilter abstraction and packages org.jacoco.core.internal.analysis.filter and org.jacoco.core.test.filter.

Regarding performance we can do a simple Benchmark by e.g. analyzing rt.jar and see the extra cost.

There won't be rt.jar anymore starting from JDK 9, but this does not prevent us from tests of rt.jar from older versions.

Even if we don't make the filtering configurable we need a simple way to do comparisons in future.

Agree. So will work on this.

@Godin Thx! Regarding performance tests I was thinking about a separate Maven project which analyzes some bigger artifacts from the Maven repo (e.g. Lucene, Tomcat).

@Godin Is this closed by #735 ?

@marchof yes! 🎉