Please review fix for 7146424: Wildcard expansion for single entry classpath

[David Holmes]

On 31/07/2012 7:32 AM, Kumar Srinivasan wrote:
> Hi David,
>> Hi Kumar,
>>
>> Don't meant to be difficult but a global int i shared across the two
>> loops is not what I was suggesting rather that the loop variable be
>> declared in the loop eg:
>>
>> 114 for (int i = 0 ; i < margc ; i++) {
>> 115 margv[i] = stdargs[i].arg;
>> 116 }
>>
>> then you can change 117 to not use i
>>
>> 117 margv[i] = NULL;
>>
>> becomes
>>
>> margv[margc] = NULL;
>
> Ah!, oh sorry that won't fly this is c code :-( , not c++, MSC does not
> allow it.

Wow that is unbelievable! for-loop variable declarations are part of C99 :(

David
-----

> Kumar
>
>
>>
>> David
>>
>> On 31/07/2012 6:11 AM, Kumar Srinivasan wrote:
>>> Hi David,
>>>
>>> Here are the changes you suggested, removed 2 scopes in
>>> main.c
>>>
>>> The delta from last reviewed revision:
>>> http://cr.openjdk.java.net/~ksrini/7146424/webrev.1/webrev.delta/index.html
>>>
>>>
>>> The full webrev:
>>> http://cr.openjdk.java.net/~ksrini/7146424/webrev.1/index.html
>>>
>>> Thanks
>>> Kumar
>>>
>>>> Hi Kumar,
>>>>
>>>> I'm always uncomfortable when I see common code that is effectively a
>>>> no-op on all but one platform - as it means it isn't really common, we
>>>> just factored it that way. But I'm not vehemently opposed. :)
>>>>
>>>> David
>>>>
>>>> On 30/07/2012 10:32 PM, Kumar Srinivasan wrote:
>>>>> HI David,
>>>>>
>>>>>> I can't comment on the details of the actual expansion logic, nor the
>>>>>> tests.
>>>>>
>>>>> Akhil and I have gone over this.
>>>>>
>>>>>>
>>>>>> Looking at the overall structure I'm still unclear why more of this
>>>>>> isn't just hidden in win32 only files. Why do the new JLI_* methods
>>>>>> have to be JLI methods? I would have hoped that everything could be
>>>>>> hidden/handled inside CreateApplicationArgs/
>>>>>
>>>>> We need JLI_* methods because all of the launcher's implementation
>>>>> is in
>>>>> the library libjli.so on Unix
>>>>> and on windows, jli.dll.
>>>>>
>>>>> Now, main.c is a common stub which provides the main/Winmain for
>>>>> all the
>>>>> launchers, meaning java
>>>>> as well as javac, javap etc. therefore main.c is linked with libjli.so
>>>>> and all of them call into the JLI_Launch an
>>>>> entry point which starts the launch, with the argc, argv, Note that
>>>>> substantial argument processing is performed much before we reach
>>>>> CreateApplicationArgs
>>>>>
>>>>> Since this expansion is required before we call into JLI_Launch, we
>>>>> need
>>>>> to call JLI_CmdToArgs
>>>>> specifically for Windows, for our parsed arguments, so that we can
>>>>> substitute argc, argv with our parsed version.
>>>>>
>>>>> Does that answer your question ? So do you have reservations about
>>>>> exposing the JLI_* interfaces,
>>>>> I think it is possible to encapsulate these completely within the jli
>>>>> library, thus not needing to
>>>>> export those interfaces, but that will complicate the logic within
>>>>> JLI_Launch, requiring platform
>>>>> specific expansions functions. I think what we have now is a lot
>>>>> cleaner.
>>>>>
>>>>>>
>>>>>> One specific comment:
>>>>>>
>>>>>> share/bin/main.c:
>>>>>>
>>>>>> 99 #ifdef _WIN32
>>>>>> 100 {
>>>>>> 101 int i = 0;
>>>>>> 102 if (getenv(JLDEBUG_ENV_ENTRY) != NULL) {
>>>>>> 103 printf("Windows original main args:\n");
>>>>>> 104 for (i = 0 ; i < __argc ; i++) {
>>>>>> 105 printf("wwwd_args[%d] = %s\n", i, __argv[i]);
>>>>>> 106 }
>>>>>> 107 }
>>>>>> 108 }
>>>>>>
>>>>>> Does MSC not permit declaration of i inside the for loop? It avoids
>>>>>> the need for the extra scope.
>>>>> MSC permits, there are two uses of the for-loop counters, I guess I
>>>>> can
>>>>> create one variable to handle
>>>>> both, and eliminate the scopes.
>>>>>
>>>>> Kumar
>>>>>
>>>>>>
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>> On 27/07/2012 10:41 PM, Kumar Srinivasan wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review the fix
>>>>>>> http://cr.openjdk.java.net/~ksrini/7146424/webrev.0/
>>>>>>>
>>>>>>> to address:
>>>>>>> 7146424: Wildcard expansion for single entry classpath
>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7146424
>>>>>>> and
>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7167744
>>>>>>>
>>>>>>>
>>>>>>> Notes:
>>>>>>> a. cmdtoargs.c will be pushed as a separate changeset using a
>>>>>>> separate CR
>>>>>>> and with contributor attribution to akhil.arora at oracle.com
>>>>>>>
>>>>>>> b. src/solaris/bin/java_md.c is a redundant file and will be
>>>>>>> removed,
>>>>>>> webrev for whatever reason is not reporting it.
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Kumar
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>
>