Review request for JMC-6548: Duplicated Flags rule somewhat naive

[Florian David]

Here is the new version of the patch.

JIRA: https://bugs.openjdk.java.net/browse/JMC-6548
Webrev: http://cr.openjdk.java.net/~fdavid/JMC-6548/webrev.1/

Cheers,
Florian


On Tue, Aug 6, 2019 at 6:55 PM Florian David <florian.david at datadoghq.com>
wrote:

>
>
> On Tue, Aug 6, 2019 at 5:53 PM Henrik Dafgård <hdafgard at gmail.com> wrote:
>
>> Hi Florian,
>>
>> The split needs to have that limit there due to the experimental
>> arguments with options, i.e. -XX:FlightRecorderOptions=stackdepth=128. With
>> this change we would get inconsistent results there instead as the argument
>> would be seen as FlightRecorderOptions=stackdepth=128 instead of
>> FlightRecorderOptions.
>>
>> Oh right, I did not think about these ones.
>
>
>> Also, if an argument matches one in the VERBATIM array it should only be
>> compared using the full argument and we won't even run that split on that
>> argument. If we want to compare multiple -javaagent commands that share
>> jarpaths but might have different options that could be contradictory I
>> think we need to have a more detailed solution, i.e. have a check for
>> "-javaagent" as a prefix instead of adding it to the VERBATIM array and
>> handling those arguments specially.
>>
>>
> After an offline discussion with Henrick, we think it's better to display
> a message in case there is duplicate agents ("duplicate" here meaning
> agents with the same jar) even if given options are different. This is
> something the current patch does not catch, I'll update it accordingly.
>
>
>>
>> Cheers,
>> Henrik Dafgård
>>
>>
>> On Tue, 6 Aug 2019 at 14:41, Florian David <florian.david at datadoghq.com>
>> wrote:
>>
>>> Hi,
>>>
>>> Please find attached a webrev to solve JMC-6548.
>>>
>>> I have also corrected a small bug on line 123 (String[] split =
>>> fullArgument.split("[=:]",
>>> 2);). In case fullArgument contains more than one ':' (for instance
>>> "-javaagent:C:/mypath"), split[1] does not contain the exact option
>>> argument (in this case only "C"), which could give inconsistent results.
>>>
>>> JIRA: https://bugs.openjdk.java.net/browse/JMC-6548
>>> Webrev: http://cr.openjdk.java.net/~fdavid/JMC-6548/webrev.0/
>>>
>>> Thanks,
>>> Florian
>>>
>>