RFR v5 - 8027634: Support @argfiles for java command-line tool

[Mandy Chung]

> On Aug 16, 2015, at 4:51 PM, Henry Jen <henry.jen at oracle.com> wrote:
> 
> 
>> args.c
>> You have #define NOT_FOUND -1 but NOT_FOUND is not used else where.
>> 
> 
> Used right under to initialize firstAppArgIndex, and should be used in following statement you shown.

Please change that.

> 
>> void JLI_InitArgProcessing(jboolean isJava, jboolean disableArgFile) {
>>     // for tools, this value remains 0 all the time.
>>     firstAppArgIndex = isJava ? -1 : 0;
>> 
>> If I understand correctly, firstAppArgIndex set to 0 means that @argfile is disabled.  On the other hand, ENABLE_ARG_FILES is also the flag to enable JDK tools to enable @argfile (disabled by default).  It seems to me that you no longer needs isJava parameter here.
> 
> firstAppArgIndex is the first index of user’s application argument, has nothing to do with disable @argfile. This value remains 0 for launcher-based tools, and returned by JLI_GetAppArgIndex().
> 
> A tools can have ENABLE_ARG_FILES to enable argument expansion, but we still need to know it’s a launcher-based tool.


OK.   firstAppArgIndex is not used in the parsing.


> 
>> Might be killSwitchOn can be replaced with firstAppArgIndex == 0 case (not sure - will let you think about that.)   killSwitchOn is unclear what it means;  maybe renaming it to disableArgFile?
>> 
> 
> As explained earlier, firstAppArgIndex is different. We can rename killSwitchOn, it was clear as we discussed kill switch, the -X:disable- at files option. disableArgFile is used as the function argument, thus I left them as is.
> 

It’s better to rename killSwitchOn to match what it means.

>> You may have to try building one tool with ENABLE_ARG_FILES to verify the change.
> 
> java is built with the flag. Others are not. I tested with javac before the flag is reversed, so we know the flag is working.

That’s good.  I re-read that piece of code and that looks fine.


> 
>> It might be useful to add a few negative tests to sanity check especially on the quotation.
>> 
> 
> Make sense. Do you mean test cases that will fail launch of java?

Yes.

Mandy