Thanks Sherman!
On 05.01.2015 22:10, Xueming Shen wrote:
Just wonder if we really need that "inQuotes" logic here? A
straightforward approach might
be "every time you have a quote, skip everything until you hit another
one, when counting,
or copy everything into the buffer until hit another one, when copying" ?
I agree it would work, but, in my opinion, it would be a bit more
complicated.
The counting loop would look something like this:
------------------------------------
outerLoop: for (int i = 0; i < ldLen; ++i) {
char ch = ldPath.charAt(i);
if (mayBeQuoted && ch == '\"') {
thereAreQuotes = true;
for (++i; i < ldLen; ++i) {
if (ldPath.charAt(i) == '\"') {
continue outerLoop;
}
}
break; // unpaired quote
} else if (ch == ps) {
psCount++;
}
}
------------------------------------
which is 3 lines longer, comparing to the loop with inQuotes flag.
Since you are making copy (into the temporary buffer) anyway for
"quote" case, another
approach might be to pre-scan the input propName first for quote, if
there is one, make
a clean copy of the propName without those quotes, then go to the
original implementation.
Unfortunately, it wouldn't work in those rare cases, where the PATH
contains a quoted path with semicolon in it.
Windows allows semicolon in the file names, and it seems to be the only
reason to allow quoting the PATH entries.
Sincerely yours,
Ivan
This logic can be implemented at the very beginning as
private static String[] intializePath(String propname) {
if (ClassLoaderHelper.allowsQuotedPathElements) {
...
}
... // existing implementation
}
-Sherman
On 01/05/2015 10:10 AM, Roger wrote:
Hi Ivan,
Looks pretty good.
Just a thought on ClassLoader:1754; If you avoid the local for
ClassLoaderHelper.allowsQuotedPathElements
then the compiler can optimize the code and may do dead code
elimination since it is a final static boolean.
On Mac and Unix it reduces to just the original code.
(ClassLoaderHelper seems like a weak case for a separate class, IMHO)
$.02, Roger
On 1/5/2015 12:49 PM, Ivan Gerasimov wrote:
Hi Roger!
I've updated the webrev similarly to what you've suggested:
1) Presence of quotes is checked when counting the separators.
2) If quotes weren't found, we'll execute the same optimized loop as
for Unix.
http://cr.openjdk.java.net/~igerasim/8067951/4/webrev/
Sincerely yours,
Ivan
On 05.01.2015 18:40, Roger wrote:
Hi Ivan,
For this small difference in the implementations, I'd recommend
against having
two different source files. The path initialization function is a
one time function and
the performance improvement is not significant.
I'd suggest a few comments on your 2nd version[1].
- The windows check should check the system property or other
definitive os check
and could be better expressed (as Alan suggested) as quotesAllowed.
- in the loop testing for the quote (") can come before the
quotesAllowed check to
speed things up (no need to check if they are allowed if they do
not occur).
This code is unlikely to be executed enough times to optimized.
Roger
[1] http://cr.openjdk.java.net/~igerasim/8067951/2/webrev/
On 1/4/2015 3:23 PM, Ivan Gerasimov wrote:
On 04.01.2015 22:50, Alan Bateman wrote:
On 03/01/2015 17:39, Ivan Gerasimov wrote:
Currently, there are tree variants of ClassLoaderHelper: for
Windows, for Unix and for MacOS.
We have to either duplicate code in Unix and MacOS realizations,
or introduce another Helper class for initializing paths only,
which would have only two realizations: for Windows and all Unixes.
When I made the comment then I was thinking of a method such as
allowsQuotedPathElements (or a better name) that returns a
boolean to indicate if quoting of path elements is allowed or
not. That would abstract the capability a bit without needing to
do isWindows checks.
Ah, I see.
Though, not needing to check for quotes allows a bit more
efficient implementation, so splitting the code for different
platforms may also make sense.
I did it with another helper class in this webrev:
http://cr.openjdk.java.net/~igerasim/8067951/3/webrev/
Sincerely yours,
Ivan
-Alan.
