RFR 7902143: Add an ANC filter that takes method list as input
Alexey Fedorchenko
alexey.fedorchenko at oracle.com
Tue Apr 10 21:54:22 UTC 2018
Hello!
Looks good to me, thank you!
—Alexey
> On Apr 9, 2018, at 11:51 AM, Alexandre (Shura) Iline <alexandre.iline at oracle.com> wrote:
>
> New version: http://cr.openjdk.java.net/~shurailine/7902143/webrev.02/ <http://cr.openjdk.java.net/~shurailine/7902143/webrev.02/>
>
> More inline.
>
>> On Apr 6, 2018, at 7:15 PM, Alexey Fedorchenko <alexey.fedorchenko at oracle.com <mailto:alexey.fedorchenko at oracle.com>> wrote:
>>
>> Hello!
>>
>> Please, take a look at these issues:
>>
>> - typo in ListANCFilter javadoc: “of" -> “or”.
>>
>> - empty lines in the end of RepGen.
>>
>> - imports in RepGen is something in the middle between the old and “optimized" by IDE (we need to chose any side here)
>
> I have reverted to the old order. You are right that a choice of IDE should not dictate order of imports.
>
>>
>> - typo in the class name: “ParameterisedAncFilter" -> “ParameterizedAncFilter”
>
> Turns out, “parameterised” is British and “parameterized” is American. Who knew? Fixed that.
>
>>
>> - The logic if condition in the RepGen is incorrect:
>> The code verifies if the filter is ParameterisedAncFilter and if it is not - throws an exception.
>> That breaks all default not parameterized filters.
>
> Yeah, sorry about that. Last minute changes ... That is why tests are needed. Fixed.
>
>>
>> - ListANCFilterTest uses the Files.newBufferedWriter(file) method from 1.8, but minimal version is 7:
>> https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#newBufferedReader-java.nio.file.Path- <https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#newBufferedReader-java.nio.file.Path->
>> Charset.defaultCharset() should help.
>
> I actually do not see a problem with using JDK8 for tests, but this case clearly does not justify the trouble of setting test JDK separately. Fixed.
>
> Shura
>
>>
>> —Alexey
>>
>>
>>> On Apr 6, 2018, at 6:44 PM, Alexandre (Shura) Iline <alexandre.iline at oracle.com <mailto:alexandre.iline at oracle.com>> wrote:
>>>
>>> Updated to the last source and fixed the “all” filters problem.
>>>
>>> New web rev: http://cr.openjdk.java.net/~shurailine/7902143/webrev.01/ <http://cr.openjdk.java.net/~shurailine/7902143/webrev.01/>
>>>
>>> Thank you
>>>
>>> Shura
>>>
>>>> On Apr 6, 2018, at 12:32 PM, Alexey Fedorchenko <alexey.fedorchenko at oracle.com <mailto:alexey.fedorchenko at oracle.com>> wrote:
>>>>
>>>> The new “parameterized” filters should be excluded from “all” filters (because there is no parameter for these filters in this case).
>>>>
>>>> The idea to fail if default filter is not found looks reasonable (if the filter is listed in the loader, it should be present).
>>>> I am not sure that the filter should fail full report generation process if there is an error (but unexpected result is also not the best
>>>> choice), anyway the current default anc filters are simple and do not log/throw any errors and we can have this “full fail” rule.
>>>>
>>>> —Alexey
>>>>
>>>>
>>>>> On Apr 5, 2018, at 1:57 PM, Alexandre (Shura) Iline <alexandre.iline at oracle.com <mailto:alexandre.iline at oracle.com>> wrote:
>>>>>
>>>>> I will fix the web rev to the latest code, thanks.
>>>>>
>>>>> I do not think you need to commit for me - I will commit myself when you are OK with the changes, putting you as a reviewer.
>>>>>
>>>>> Are you OK with the actual code changes, though?
>>>>>
>>>>> Shura
>>>>>
>>>>>
>>>>>> On Apr 5, 2018, at 11:36 AM, Alexey Fedorchenko <alexey.fedorchenko at oracle.com <mailto:alexey.fedorchenko at oracle.com>> wrote:
>>>>>>
>>>>>> Hello!
>>>>>>
>>>>>> Thanks for the patch! I will review the content of the changes.
>>>>>>
>>>>>> Please, look at these items:
>>>>>> - the changes for the build.xml looks like a diff to your local copy and not to the latest repo state:
>>>>>> http://hg.openjdk.java.net/code-tools/jcov/file/391790f13cfd/build/build.xml#l165 <http://hg.openjdk.java.net/code-tools/jcov/file/391790f13cfd/build/build.xml#l165>
>>>>>> (The target was set to 1.7 long time ago)
>>>>>> - please, send a patch not a diff (I will be able to push it from your name)
>>>>>> - do you plan to provide any execution mechanism for the /test/unit ?
>>>>>>
>>>>>> Thank you.
>>>>>>
>>>>>> —Alexey
>>>>>>
>>>>>>> On Apr 4, 2018, at 3:44 PM, Alexandre (Shura) Iline <alexandre.iline at oracle.com <mailto:alexandre.iline at oracle.com>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> On Apr 4, 2018, at 1:25 PM, Alexandre (Shura) Iline <alexandre.iline at oracle.com <mailto:alexandre.iline at oracle.com>> wrote:
>>>>>>>>
>>>>>>>> Hi.
>>>>>>>>
>>>>>>>> Please take a look on this new ANC filter:
>>>>>>>> http://cr.openjdk.java.net/~shurailine/7902143/webrev.00/ <http://cr.openjdk.java.net/~shurailine/7902143/webrev.00/>
>>>>>>>>
>>>>>>>> Please notice that beyond actually adding the functionality I am suggesting to
>>>>>>>> 1. allow to use Java 7 features
>>>>>>>> 2. change RepGen behavior in regards to errors in ANC filters. It should not just print a message - it should fail.
>>>>>>>>
>>>>>>>> Shura
>>
>
More information about the jcov-dev
mailing list