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

[Henry Jen]

Thanks for reviewing, comment inline below,

> On Aug 14, 2015, at 4:07 PM, Mandy Chung <mandy.chung at oracle.com> wrote:
> 
> 
>> On Aug 14, 2015, at 1:10 PM, Henry Jen <henry.jen at oracle.com> wrote:
>> 
>> Hi,
>> 
>> Another minor revision address comments, no real behavior changes except use JLI_StrCmp instead of JLI_StrCCmp in checkArg().
>> 
>> http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v5/webrev/
> 
> main.c
>   JLI_PreprocessingArg returns NULL if not @argfile
> 
> Would it be better to return JLI_List containing one element as argv[i]?  We want to avoid new/free JLI_List for every argument and maybe a preallocated reusable copy for single element list to use (non-growable)?

Only argument with @prefix will be expanded, for any other cases, the function return NULL. This avoid any copy when not necessary. Regular argument is left alone, so that caller will just use the original value.

> 
> 140                 // Shallow free, we reuse the string to avoid copy
> 141                 JLI_MemFree(argsInFile->elements);
> 142                 JLI_MemFree(argsInFile);
> 
> What about adding JLI_List_free(JLI_List sl, boolean shadow)?  This would be useful for the reusable single element list. Same comment applied to cmdtoargs.c
> 

Thought of that, but decided to keep in place for clarity. When move into a function, then we want to check for NULL, and ask  how shallow the free is, do we keep the array but free the list or both?

> 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.

>  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.

>  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.

> 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.

> 
> JLI_PreprocessArg - as mentioned in the comment above, I suggest to have JLI_PreprocessArg to always return a non-null JLI_List and perhaps renaming the method to JLI_ExpandArgumentList
> 

NULL is better than non-NULL copy so that we don’t need to copy the argument or check content when it’s not @ prefixed.
The function was named JLI_ExpandArgumentList, later renamed to current form as it also check each argument to determine the location of first user argument for main class.

>            // We can not just update the idx here because if -jar @file
>            // still need expansion of @file to get the argument for -jar
> 
> I only skimmed on the tests.
>    ArgFileSyntax.java line 167-170: nit: indentation should be 4-space
> 

You are right, thanks. When I was making those changes, I noted the editor changed something, but didn’t realize it’s the indentation.

> 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?

> TestHelper.java has no change - should be reverted.
> 

It has two minor indentation clean up, I can revert them, but think since we were here, perhaps just fix it.

Cheers,
Henry