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

[David Holmes]

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;

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
>>>>>
>>>>>
>>>>>
>>>
>