> On Aug 14, 2015, at 1:10 PM, Henry Jen <henry....@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)?
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
args.c
You have #define NOT_FOUND -1 but NOT_FOUND is not used else where.
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. 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?
You may have to try building one tool with ENABLE_ARG_FILES to verify the
change.
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
// 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
It might be useful to add a few negative tests to sanity check especially on
the quotation.
TestHelper.java has no change - should be reverted.
Mandy
