> On Aug 16, 2015, at 4:51 PM, Henry Jen <[email protected]> wrote:
>
>
>> 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.
Please change that.
>
>> 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.
OK. firstAppArgIndex is not used in the parsing.
>
>> 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.
>
It’s better to rename killSwitchOn to match what it means.
>> 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.
That’s good. I re-read that piece of code and that looks fine.
>
>> 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?
Yes.
Mandy
