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