Please review fix for 7146424: Wildcard expansion for single entry classpath (original) (raw)

David Holmes david.holmes at oracle.com
Mon Jul 30 21:43:41 UTC 2012

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 JLILaunch 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 JLILaunch, we need to call JLICmdToArgs 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 JLILaunch, 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(JLDEBUGENVENTRY) != NULL) { 103 printf("Windows original main args:\n"); _104 for (i = 0 ; i < argc ; i++) { _105 printf("wwwdargs[%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/viewbug.do?bugid=7146424 and http://bugs.sun.com/bugdatabase/viewbug.do?bugid=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/javamd.c is a redundant file and will be removed, webrev for whatever reason is not reporting it. Thanks Kumar

More information about the core-libs-dev mailing list