Re: 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.ar...@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