RFR: JDK-8243417 Clean up com.sun.tools.javac.main.CommandLine

[Pavel Rappo]

Jon,

It is a tiny little internal API that we are talking about. Thus, it probably isn't worth wasting too much energy reviewing that changeset. I'm fine with the proposed change. However, I want to clarify my argument as I felt there was a misunderstanding.

At no time was I talking about performance. I was talking about convenience, API ergonomics. Say, I'm a client of that `parse` method. What I (typically) have is an array and what I want back is a list. This is because the list has a richer API and that's what I want to use. Before the change, the `parse` method accepted an array and spat back also an array. Since I wanted to use a list, I had to convert that array to a list myself. Now comes the change and says, "Alright, you'll get your list, but I will no longer accept arrays!" So for me as a client, nothing changes much. I still have to perform a conversion. It's just that this time it's not for the result but for the argument. What would make more sense, in my opinion, is if the API kept accepting an array, but began returning a list.

Once again, I'm fine with that change the way it is. Just wanted to clarify my point.

-Pavel

> On 25 Apr 2020, at 15:38, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
> 
> Pavel,
> 
> Performance of this method is not an issue, when you compare the amount of work done by this method compared to the total work done by javac and javadoc. That being said, the new code is better than the old since when using the array wrappers in the old code, internally, the array was converted to a list, a new list was created with the expanded contents, the list was converted to an array, and then the client converted that array back to a list, for a total of 3 conversions. The new code just has a single conversion. I think that's an acceptable cost for a simplified API.
> 
> -- Jon
> 
> 
> On 4/24/20 1:44 PM, Pavel Rappo wrote:
>> Hey Jon,
>> 
>> My only comment is that while eliminating the need for conversion in one place, the change introduces it to another place, effectively discounting the benefits of the change.
>> 
>> Maybe it's okay for that method to consume String[] and produce List<String>. Or at least to have that as an option. `public static void main(String[] args)` accepts array, while the code it calls accepts List. We can't change the signature of the `main` method. And the clients want to work with List. Someone has to do the job of conversion. Why not CommandLine.parse? Typically that method is the first method to consume the arguments from the `main` method anyway.
>> 
>> -Pavel
>> 
>>> On 24 Apr 2020, at 19:32, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>> 
>>> javac folk,
>>> 
>>> Please review a small cleanup to the code to handle @file expansion.
>>> 
>>> Pavel recently noted that the consumers of CommandLine.parse all eventually wanted Lists, which is the primary internal form inside CommandLine, but that the clients were all using array-based wrappers, causing unnecessary conversions between the two forms.
>>> 
>>> The fix is to have the clients use the List-based methods, and to remove the legacy array-based methods.  In some of the javac code, Iterable<String> is used to store the result, to avoid confusion with javac's internal List class.
>>> 
>>> -- Jon
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8243417
>>> Webrev: http://cr.openjdk.java.net/~jjg/8243417/webrev.00/
>>>