RFR v5 - 8027634: Support @argfiles for java command-line tool
Mandy Chung
mandy.chung at oracle.com
Fri Aug 14 23:07:52 UTC 2015
> 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)?
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
More information about the core-libs-dev
mailing list