RFR v4 - 8027634: Support @argfiles for java command-line tool

[Kumar Srinivasan]

Henry,

<trimmed the cc: list>

I am still not complete, but here are some more comments and specifics.

args.c

1.
-static int firstAppArgIndex = -1;

+#define NOT_FOUND  -1;
+static int firstAppArgIndex = NOT_FOUND;

2.
     // Any @argument comes in here is an argument as is.
     // Expansion should had done before checkArg called.

I think you are trying to say
    // All arguments arrive here must be a launcher argument,
    // ie. by now, all argfile expansions must have been performed.


3. void JLI_InitArgProcessing(jboolean isJava, jboolean disableArgFile) {
     // No expansion for relaunch


The launcher might re-exec itself, this might happen under certain
circumstances on *nix, is the above logic to prevent subsequent
expansion on relaunch ? However when a re-exec occurs, the
expanded args will be sent to the second invocation of the launcher.

So what does "relaunch" mean ? in your comment ?

ArgFileSyntax.java:

usage of ArrayList vs. Arrays will simplify things.

     // arg file content,  expected options
     static String[] testCaseArrays[][] = {
         { // empty file
             {}, {}
         },

...
     public List<List<List<String>>> loadCases() {
         List<List<List<String>>> rv = new ArrayList<>();
         for (String[][] testCaseArray : testCaseArrays) {
             List<List<String>> testCase = new ArrayList<>(2);
             testCase.add(Arrays.asList(testCaseArray[0]));
             testCase.add(Arrays.asList(testCaseArray[1]));
             rv.add(testCase);
         }
         // long lines
.....
     @Test
     public void testSyntax() throws IOException {
         List<List<List<String>>> allCases = loadCases();
         for (List<List<String>> testCase : allCases) {
             verifyParsing(testCase.get(0), testCase.get(1));
         }
     }


ArgsFileTest.java:

use case coverage looks great, still haven't finished with this
yet.

Kumar

>> On Aug 12, 2015, at 7:55 AM, Kumar Srinivasan <kumar.x.srinivasan at oracle.com> wrote:
>>
>> Henry,
>>
>> Generally looks good here are some comments, on my initial
>> pass, I am not fully done with args.c I will look at this some
>> more later today or tomorrow.
>>
>> 4.)
>> expectingNoDash is expectingMain right ? if so I would rename it so.
>>
> Not really, it is expecting a argument without dash, such as -cp or -classpath.
>
>>
>> TestHelper.java:
>>
>> Why add findInOutput method ? this seems to be identical
>> to "matches" at line 606, which does exactly that.
>>
> To return a string, this is used when trying to match a pattern and get information from that line, I used to get the starting index for an argument in verifyOptions.
>
> Guess I can remove that now as each use of verifyOptions all starts from 1.
>
>> ArgFileSyntax.java
>> Nice, but I think the creation and storage of test case can be simplified,
>> basically loadCases(), also please avoid  Xbootclasspath. The trouble
>> with these tests which operate loops, are very painful to debug
>> and isolate a failing case,  on the night before GA.
>>
> I wrote that because I start with testng as a DataProvider, we can change that.
>
>> Can we do something like this.....
>>
>> Map<String, List<String>> tests = new ArrayList<>();
>> tests.put("testing quotes", Array.asList({{...}, {....}});
>> tests.add("no recurse expansion", Array.asList({{...}, {....}});
>>
>> The execute each test, with a description of what the test is doing.
>> This way if a test fails its easy to zero in on the failing test.
>>
> verifyOutput will print out what line is not matching, so it’s pretty easy to identify which case failed.
>
> Cheers,
> Henry
>