Thanks for reviewing, comment inline below,
> On Aug 14, 2015, at 4:07 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>
>
>> 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)?
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-@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
