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

[Mike Duigou]

On Jul 30 2012, at 14:43 , David Holmes wrote:

> 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

Sadly Microsoft does not and will will not be providing a C99 compiler.

http://herbsutter.com/2012/05/03/reader-qa-what-about-vc-and-c99/

Mike

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