Re: RFR: Export InitializeEncoding for JVM access

[Andrew Leonard]

Many thanks Roger, much appreciated.
Cheers
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:   Roger Riggs 
To: Andrew Leonard 
Cc: core-libs-dev@openjdk.java.net
Date:   09/04/2018 19:52
Subject:Re: RFR: Export InitializeEncoding for JVM access



Hi Andrew,

ok,  it turns out the prototype in jni_util.h also needs to be updated to 
be consistent for Windows and Solaris.
And no surprise, there are no test failures.

I put an updated webrev in:
  http://cr.openjdk.java.net/~rriggs/webrev-encoding-8201246/

If there are no more comments, I'll commit tomorrow.

Regards, Roger


On 4/9/2018 8:57 AM, Andrew Leonard wrote:
Thanks for the feedback Roger, 
Yes, exporting the InitializeEncoding entry point would make sense, 
keeping it consistent with Canonicalize(). I have attached the new patch 
below: 
Many thanks 
Andrew 


diff --git a/src/java.base/share/native/libjava/jni_util.c 
b/src/java.base/share/native/libjava/jni_util.c 
--- a/src/java.base/share/native/libjava/jni_util.c 
+++ b/src/java.base/share/native/libjava/jni_util.c 
@@ -774,8 +774,10 @@ 
 return newSizedStringJava(env, str, len); 
 } 
  
-/* Initialize the fast encoding from the encoding name. */ 
-void 
+/* Initialize the fast encoding from the encoding name. 
+ * Export InitializeEncoding so that the VM can initialize it if 
required. 
+ */ 
+JNIEXPORT void 
 InitializeEncoding(JNIEnv *env, const char *encname) 
 { 
 jclass strClazz = NULL; 



Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:Roger Riggs  
To:Andrew Leonard  
Cc:core-libs-dev@openjdk.java.net 
Date:06/04/2018 15:39 
Subject:Re: RFR: Export InitializeEncoding for JVM access 



Hi Andrew,

Would it be sufficient to export the InitializeEncoding entry point?  

Introducing a JNU_xx name may imply a level of support that is unwarranted 
since
it is part of system initialization and may change as needed by the 
implementation.
Simply exporting it would put it on par with Canonicalize.

Thanks, Roger

p.s. Tracking issue: https://bugs.openjdk.java.net/browse/JDK-8201246

On 4/5/2018 5:49 AM, Andrew Leonard wrote: 
Hi Roger, 
The OpenJ9 VM implementation provides its own java.lang.System, which 
performs similar type early-on VM initialization to the OpenJDK core 
library classes. The basic reason for invoking the method is to initialize 
the platform encoding either called from a related core library method, 
eg.initProperties(), or from the VM in the early stages of initialization. 


My suggested comparison with canonicalize was based on it's indicated 
usage: 
/* 
 * Export the platform dependent path canonicalization so that 
 * VM can find it when loading system classes. 
 * This function is also used by the instrumentation agent. 
 */ 
extern int canonicalize(char *path, const char *out, int len); 

JNIEXPORT int 
Canonicalize(JNIEnv *unused, char *orig, char *out, int len) 
{ 
/* canonicalize an already natived path */ 
return canonicalize(orig, out, len); 
} 


Many thanks, 
Andrew 


Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 


Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU 





Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU




Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: RFR 8201348, ProblemList update for bugid associated with SSLSocketParametersTest.sh

[Lance Andersen]

+1
> On Apr 9, 2018, at 11:15 PM, Felix Yang  wrote:
> 
> Hi,
> 
> please review a minor change on the associated bug id in ProblemList.txt.
> 
> Bug:
> 
> https://bugs.openjdk.java.net/browse/JDK-8201348
> 
> Patch:
> 
> diff -r f088ec60bed5 test/jdk/ProblemList.txt
> --- a/test/jdk/ProblemList.txt  Mon Apr 09 10:39:29 2018 -0700
> +++ b/test/jdk/ProblemList.txt  Mon Apr 09 19:26:47 2018 -0700
> @@ -778,7 +778,7 @@
> 
>  com/sun/jndi/ldap/LdapTimeoutTest.java 8151678 linux-all
> 
> -javax/rmi/ssl/SSLSocketParametersTest.sh 8194663 generic-all
> +javax/rmi/ssl/SSLSocketParametersTest.sh 8162906 generic-all
> 
>  
> 
> 
> Thanks,
> 
> Felix
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR [10] 8194554: filterArguments runs multiple filters in the wrong order

[Aleksey Shipilev]

On 04/10/2018 08:42 AM, Alan Bateman wrote:
> On 09/04/2018 18:58, Paul Sandoz wrote:
>> I am supportive of this change (the risk to impacting order-dependent 
>> stateful MH filter code is
>> smaller than the risk of hitting a string concatenation bug). (We erred on 
>> the side of this being
>> a bug and not being a spec change given the pseudo-code in the Java doc.)
>>
>> IIRC this will require a nod of approval from the project lead.
>>
>> Paul.
> I think this is a CSR for Java SE 11 first as it adds a testable assertion to 
> the spec and also
> changes existing behavior.

Ok, fine! Paul, can you submit the CSR for this?

-Aleksey

Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use

[Roger Riggs]

Hi Hamlin,

Looks good,

Thanks for the updates.

On 4/9/2018 10:49 PM, Hamlin Li wrote:


On 10/04/2018 3:09 AM, Roger Riggs wrote:

Hi Hamlin,

Hi Roger,

Thank you for review.


Great, the new busy port algorithm looks better.
The port assigned will always be one that was available and is now 
busy to prevent the registry creation.


As expected, there is a small window  between the (try-with-finally) 
close of the server socket channel
and the 2nd createReg.  But that can't be avoided.  If the port is 
re-used in that gap
the test will fail.  (And the exception handler at 77 will see an 
in-use and retry).

Yes, you're right.


67: The exception being caught is already one thrown by 
TestLibrary.bomb so it would be
cleaner to just re-throw e;  or better yet, just don't bother to 
catch the exception.

That exception should cause the test to fail.
It's to catch IOException by ServerSocketChannel.open, bind. Seems 
it's a little confusing, so I declare at main function to throw 
IOException.


It may be personal coding style but the createReg method with a 
boolean flag to suppress
the exception is just more confusing that putting the code in-line in 
the two places it is used.
I think it's mainly because of original parameter name remoteOK, so 
rename it as expectException, and move it to the last parameter.


new webrev is updated in place:
http://cr.openjdk.java.net/~mli/8188897/webrev.02/

Thank you
-Hamlin


Thanks, Roger


On 4/8/2018 4:10 AM, Hamlin Li wrote:

Hi Roger,

I have changed to not use RegistryVM at all, please review the 
patch: http://cr.openjdk.java.net/~mli/8188897/webrev.02/


Thank you

-Hamlin


On 04/04/2018 10:15 PM, Roger Riggs wrote:

Hi Hamlin,

Reexport.java:

I think this change to use a separate process for the 2nd registry 
changes the test so that it does not
address the original test case.  The original problem was the 
incorrect retention of an object in
the object table when the create of a registry in the same process 
failed.
Finally being able to create the registry in the same process 
assured that the object was not

retained in the object table.

Go back to creating the 2nd registry in the test process.


RegistryVM.java: 2, the copyright update should be  "2017, 2018," 
range.


(I'm  really not a fan of all the RegistryVM methods with the same 
name and minimal and implicit
differences in their functions.  When reading the test, you have to 
go and read the RegistryVM method
to see what it does.  I would have preferred that the full 
createRegistryVM (out, err, options, port) method
was used directly in the tests.  In the case of a method used once, 
it is an inconvenience method, not a convenience).


The new terminate() method is quite similar to the existing 
cleanup() method which does not wait.
It would be a good cleanup to figure out if another method is 
really needed.
The override is going to change the behavior of the existing uses 
of terminate().

It should be checked that it does not break any existing uses.

178: 187: The method comments are not consistent, one says forcibly 
and the other gracefully

but both call requestExit and both call destroy().

Thanks, Roger


On 4/3/2018 11:35 PM, Hamlin Li wrote:

Hi Joe, Roger,

Thank you for reviewing, I have refactored the test more and fix 
as you suggested.


please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/

Thank you

-Hamlin


On 04/04/2018 10:42 AM, Joseph D. Darcy wrote:

Hello,

Some general comments on the coding for tests like this:

* It is preferable to avoid sleep in tests to avoid increasing 
the minimum amount of time a test takes to run. This helps limit 
the overall time it takes the test suite to run.


* If timeouts are used, it is recommend to factor the maximum 
time waited with the jtreg timeout scaling factor; I don't recall 
its exact name. In other words, many of our tests are run on 
heavily loaded systems and the tests take longer than run than on 
typical laptops and workstations so jtreg is invoked with an 
timeout scaling factor. Individual tests should be sensitive to 
this scaling factor for any internal timeout that need to be used.


HTH,

-Joe

On 4/3/2018 7:48 AM, Roger Riggs wrote:

Hi Hamlin,

Instead of a simple time delay, it would be useful to wait for 
the RegistryVM to terminate.
In killRegistry: 149,  adding subreg.waitFor() should be 
sufficient.


58: If using a 'for' loop it would be easier to understand if it 
included the usual start, increment and termination.

Instead of burying it in the exception handler.

59, 102, 104: the introduction of the kill boolean makes the 
test harder to understand and seems to be unnecessary.
 the killRegistry() method already will only kill the subprocess 
if it still is alive.


Roger

On 4/2/2018 6:33 AM, Hamlin Li wrote:

would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8188897

webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/


Thank you

-Hamlin

Re: [11] RFR: 8189784: Parsing with Java 9 AKST timezone returns the SystemV variant of the timezone

[Erik Joelsson]

This looks very good. Thanks!

(reviewed build part)

/Erik


On 2018-04-09 18:00, naoto.s...@oracle.com wrote:

Thanks, Erik. Modified GensrcCLDR.gmk as suggested:

http://cr.openjdk.java.net/~naoto/8189784/webrev.03/

Naoto

On 4/9/18 4:15 PM, Erik Joelsson wrote:

Hello Naoto,

Looks good, just a style issue.

When breaking a recipe line, please add 4 additional spaces (after 
the tab) for continuation indent [1]. In this case I would also 
advocate breaking the line sooner to make side by side comparisons 
easier in the future.


/Erik

[1] http://openjdk.java.net/groups/build/doc/code-conventions.html

On 2018-04-09 13:20, Naoto Sato wrote:

Hello,

Please review the changes to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8189784

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8189784/webrev.02/

There were two causes of the issue. One was that j.t.f.ZoneName 
contained hard coded mappings based on the old CLDR data and never 
been updated. The other reason was that CLDR's deprecated zones 
("SystemV" ones, in this case) were not properly replaced.


I am including build-dev for the review, as the change includes 
generating ZoneName mapping at the build time from the template.


Naoto

Re: RFR [10] 8194554: filterArguments runs multiple filters in the wrong order

[Paul Sandoz]

> On Apr 10, 2018, at 3:21 AM, Aleksey Shipilev  wrote:
> 
> On 04/10/2018 08:42 AM, Alan Bateman wrote:
>> On 09/04/2018 18:58, Paul Sandoz wrote:
>>> I am supportive of this change (the risk to impacting order-dependent 
>>> stateful MH filter code is
>>> smaller than the risk of hitting a string concatenation bug). (We erred on 
>>> the side of this being
>>> a bug and not being a spec change given the pseudo-code in the Java doc.)
>>> 
>>> IIRC this will require a nod of approval from the project lead.
>>> 
>>> Paul.
>> I think this is a CSR for Java SE 11 first as it adds a testable assertion 
>> to the spec and also
>> changes existing behavior.
> 

Note that the testable assertion was already in the spec, just buried in the 
normative pseudocode. So this is really about clarification from the spec 
perspective. And of course any bug fix can change behavior. In hindsight i 
would be more inclined to do a CSR if a bug fix is required to conform to 
specification.


> Ok, fine! Paul, can you submit the CSR for this?
> 

Here we go:

  https://bugs.openjdk.java.net/browse/JDK-8201371 


Kindly requesting a reviewer.

Thanks,
Paul.

Re: RFR [10] 8194554: filterArguments runs multiple filters in the wrong order

[Aleksey Shipilev]

On 04/10/2018 06:40 PM, Paul Sandoz wrote:
> Here we go:
> 
>   https://bugs.openjdk.java.net/browse/JDK-8201371
> 
> Kindly requesting a reviewer.

Not sure if that counts for CSR, but added myself as Reviewer there.

-Aleksey

Re: RFR: 8178867: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java failed to clean up files

[Andrey Nazarov]

Anyone?

> On 6 Apr 2018, at 17:10, Andrey Nazarov  wrote:
> 
> Hi,
> 
> Please review fix in Jlink test. The fix is to close the Stream which works 
> with a file system.
> 
> Review: http://cr.openjdk.java.net/~anazarov/JDK-8178867/webrev.01 
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8178867
> 
> —Thanks,
> Andrei

Re: RFR: 8178867: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java failed to clean up files

[Alan Bateman]

On 10/04/2018 19:44, Andrey Nazarov wrote:

Anyone?


On 6 Apr 2018, at 17:10, Andrey Nazarov  wrote:

Hi,

Please review fix in Jlink test. The fix is to close the Stream which works 
with a file system.

Review: http://cr.openjdk.java.net/~anazarov/JDK-8178867/webrev.01 

JBS: https://bugs.openjdk.java.net/browse/JDK-8178867

If you want, you can get rid of temporary javaFiles list and use 
.forEach(args::add) instead.


-Alan

Re: RFR: 8178867: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java failed to clean up files

[Jonathan Gibbons]

+1


On 4/10/18 11:44 AM, Andrey Nazarov wrote:

Anyone?


On 6 Apr 2018, at 17:10, Andrey Nazarov  wrote:

Hi,

Please review fix in Jlink test. The fix is to close the Stream which works 
with a file system.

Review: http://cr.openjdk.java.net/~anazarov/JDK-8178867/webrev.01 

JBS: https://bugs.openjdk.java.net/browse/JDK-8178867

—Thanks,
Andrei

Re: RFR: 8178867: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java failed to clean up files

[Andrey Nazarov]

> On 10 Apr 2018, at 11:47, Alan Bateman  wrote:
> 
> On 10/04/2018 19:44, Andrey Nazarov wrote:
>> Anyone?
>> 
>>> On 6 Apr 2018, at 17:10, Andrey Nazarov  wrote:
>>> 
>>> Hi,
>>> 
>>> Please review fix in Jlink test. The fix is to close the Stream which works 
>>> with a file system.
>>> 
>>> Review: http://cr.openjdk.java.net/~anazarov/JDK-8178867/webrev.01 
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8178867
>>> 
> If you want, you can get rid of temporary javaFiles list and use 
> .forEach(args::add) instead.
Yes. It looks cleaner. 
http://cr.openjdk.java.net/~anazarov/JDK-8178867/webrev.03
> 
> -Alan

Re: RFR: 8178867: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java failed to clean up files

[Paul Sandoz]

> On Apr 10, 2018, at 11:47 AM, Alan Bateman  wrote:
> 
> On 10/04/2018 19:44, Andrey Nazarov wrote:
>> Anyone?
>> 
>>> On 6 Apr 2018, at 17:10, Andrey Nazarov  wrote:
>>> 
>>> Hi,
>>> 
>>> Please review fix in Jlink test. The fix is to close the Stream which works 
>>> with a file system.
>>> 
>>> Review: http://cr.openjdk.java.net/~anazarov/JDK-8178867/webrev.01 
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8178867
>>> 
> If you want, you can get rid of temporary javaFiles list and use 
> .forEach(args::add) instead.
> 

Yes. Not suggesting you do this, just for educational purposes you can also do 
this (not tested) e.g.:

var argStream = Stream.of("-d", destination.toString(), "--module-source-path", 
srcpath);
try (var pathStream = Files.walk(source)) {
argStream = Stream.concat(argStream,
pathStream.map(Path::toString).filter(s -> s.endsWith(".java")));

int rc = JAVAC_TOOL.run(System.out, System.err, 
argStream.toArray(String[]::new));
Assert.assertEquals(rc, 0);
}

Paul.

Re: RFR: 8178867: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java failed to clean up files

[Alan Bateman]

On 10/04/2018 20:03, Andrey Nazarov wrote:



On 10 Apr 2018, at 11:47, Alan Bateman  wrote:

On 10/04/2018 19:44, Andrey Nazarov wrote:

Anyone?


On 6 Apr 2018, at 17:10, Andrey Nazarov  wrote:

Hi,

Please review fix in Jlink test. The fix is to close the Stream which works 
with a file system.

Review: http://cr.openjdk.java.net/~anazarov/JDK-8178867/webrev.01 

JBS: https://bugs.openjdk.java.net/browse/JDK-8178867


If you want, you can get rid of temporary javaFiles list and use 
.forEach(args::add) instead.

Yes. It looks cleaner. 
http://cr.openjdk.java.net/~anazarov/JDK-8178867/webrev.03
I agree with Paul that it would be nice to replace new 
ArrayList<>(List.of("-d", ... )) too but what you have is okay.


-Alan

Re: stripIndent() behavior

[Brian Goetz]

(now stripIndent)

I've accumulated a few questions/comments on this.

1. When choosing the amount to trim, it ought to ignore blank lines and
only-whitespace lines, right?


Seems right.


2. Is it really appropriate to automatically remove trailing whitespace?


I'm not sure about this either.  The reason that RSLs will have "extra" 
whitespace that needs to be stripped is that we want to indent the RSL 
snippet relative to the Java code (and as you point out, the IDE may do 
that automatically for us.)  But if there's trailing whitespace, its 
because the user put it there, and who is it hurting?  It might be 
significant.



3. If the input contains *any* tab characters at all (except any that are
part of the trailing whitespace), then this method cannot know that it
isn't jumbling the end result, and maybe it should just throw.
I think there's a middle ground, where it strips any common whitespace 
prefix.  So if every line starts with tab-tab-space-space, it can safely 
strip this.



5. If we do end up in a world where we have to call this for almost every
one of our tens of thousands of multi-line RSLs... is it strange that I
feel like I would prefer it was static? It seems like it would look a lot
more normal that way visually. Ugh...
I think this is likely to vary subjectively a lot.  Some people like 
that the instance method is mostly out of the way; others like the 
up-front shouting of the static method.


The reason we can't have both is then we can't resolve the method 
reference String::strip as a Function, which seems a 
useful thing to do.



On top of *that*, I have no idea what "right markers" are good for, nor
what customizing the marker choice is good for (other than creating more
needless variation between different pieces of code).



String asciiArtFTW =
`
    `  BOO  `
    `.trimMarkers("`", "`");

Re: stripIndent() behavior

[Kevin Bourrillion]

On Tue, Apr 10, 2018 at 1:50 PM, Brian Goetz  wrote:
>
>
> 3. If the input contains *any* tab characters at all (except any that are
>> part of the trailing whitespace), then this method cannot know that it
>> isn't jumbling the end result, and maybe it should just throw.
>>
> I think there's a middle ground, where it strips any common whitespace
> prefix.  So if every line starts with tab-tab-space-space, it can safely
> strip this.


My point is that this is *not* safe, if by "safe" we mean "you will see the
same relative indentation you saw in the source". *Any* tab has the
potential to suddenly be worth a different number of spaces, once some
prefix of any kind has been removed.

I would *like* to not worry about this, but we have to accept that the
programmers who will be hurt most will be the most novice.


5. If we do end up in a world where we have to call this for almost every
>> one of our tens of thousands of multi-line RSLs... is it strange that I
>> feel like I would prefer it was static? It seems like it would look a lot
>> more normal that way visually. Ugh...
>>
> I think this is likely to vary subjectively a lot.  Some people like that
> the instance method is mostly out of the way; others like the up-front
> shouting of the static method.
>

Is it just potayto, potahto, or does one of the snippets above appear a lot
more consistent with how Java code has always looked?


On top of *that*, I have no idea what "right markers" are good for, nor
>> what customizing the marker choice is good for (other than creating more
>> needless variation between different pieces of code).
>>
>>
> String asciiArtFTW =
> `
> `  BOO  `
> `.trimMarkers("`", "`");
>
>
Ha, well, I did say "good" for :-)


-- 
Kevin Bourrillion | Java Librarian | Google, Inc. | kev...@google.com

Re: stripIndent() behavior

[Brian Goetz]

On 4/10/2018 5:18 PM, Kevin Bourrillion wrote:


On Tue, Apr 10, 2018 at 1:50 PM, Brian Goetz > wrote:



3. If the input contains *any* tab characters at all (except
any that are
part of the trailing whitespace), then this method cannot know
that it
isn't jumbling the end result, and maybe it should just throw.

I think there's a middle ground, where it strips any common
whitespace prefix.  So if every line starts with
tab-tab-space-space, it can safely strip this.


My point is that this is /not/ safe, if by "safe" we mean "you will 
see the same relative indentation you saw in the source". /Any/ tab 
has the potential to suddenly be worth a different number of spaces, 
once some prefix of any kind has been removed.


That's not what I mean by safe.  I mean that one could have reasonably 
predicted what string was going to result.  Developers who use randomly 
mixed spaces and tabs do not deserve to see the same relative indentation.


While I personally dislike tabs, I accept that some people like them, 
and if used responsibly and consistently, we can all get along.  But 
using them inconsistently across lines of a multiple-line expression, or 
using them after leading spaces, are not high on my list of situations 
we should cater to.






5. If we do end up in a world where we have to call this for
almost every
one of our tens of thousands of multi-line RSLs... is it
strange that I
feel like I would prefer it was static? It seems like it would
look a lot
more normal that way visually. Ugh...

I think this is likely to vary subjectively a lot.  Some people
like that the instance method is mostly out of the way; others
like the up-front shouting of the static method.


Is it just potayto, potahto, or does one of the snippets above appear 
a lot more consistent with how Java code has always looked?


I think its pota{y,h}to as to which is more likely to trigger 
indignation at the compiler for having forced them to muck up their 
code.  And, a certain degree of fashion; chaining is "in" these days.

Re: stripIndent() behavior

[Brian Goetz]

I think this is "throwing" the baby out with the bathwater.  It is 
punishing those who can use tabs responsibly for the sins of those who 
cannot.


You have to commit three sins before you have a problem:
 - using tabs at all
 - using tabs inconsistently across the lines of a single expression;
 - using tabs after you've already used spaces on a line.

While I am sure that there are people who do so, it just seems 
unreasonable to me to throw in the presence of tabs because someone, 
somewhere, might commit these three sins together and *be confused by 
the result*.  (So, no, it's not the only option.)


Note that IDEs can also highlight code that would be inappropriately 
mangled as a result, so people learn not to commit all three of the sins 
listed.


On 4/10/2018 5:39 PM, Éamonn McManus wrote:

3. If the input contains *any* tab characters at all (except any that

are

part of the trailing whitespace), then this method cannot know that it
isn't jumbling the end result, and maybe it should just throw.

I think there's a middle ground, where it strips any common whitespace
prefix.  So if every line starts with tab-tab-space-space, it can safely
strip this.

I'm afraid that's not true. In practice if you are using tabs at all it is
very easy in many editors to end up with a mix of spaces and tabs. So you
could easy have (with 8-space tabs) one line that has 3 tabs at the start,
and another that has 2 tabs and 8 spaces. For example with Emacs you can
get this just by hitting delete after a tab and then hitting space. You
would nevertheless want stripIndent to remove the indentation from both
lines, since they look identical.

The situation is made worse by the fact that there are two common
conventions for tab width, 4 or 8.

I think the only way to avoid these problems is for stripIndent to throw if
its argument has any tabs, or at least any tabs in leading whitespace, and
provide a separate method `detab` whose argument says what the width of a
tab stop is. (Just to be sure: this method should arrange for tab stops to
be at positions 4N or 8N, where the first column is column 0. So a tab can
expand into 1 to 4 spaces, or 1 to 8 spaces.)

Then users operating in tab-free codebases can just write .stripIndent(),
and users in tab-infected codebases can write .detab(4).stripIndent().

The alternative is to expose novice users to many hours of exasperation.
Tabs are generally invisible, so you can imagine someone trying to figure
out why two lines that look exactly the same ended up treated differently.
Users may not even know there is such a thing as a tab character. (If
stripIndent throws, it should have a helpful message that suggests calling
detab(N) and that the value of n should probably be 4 or 8.)


String asciiArtFTW =
`
   `  BOO  `
   `.trimMarkers("`", "`");

I'm not sure I get that. It doesn't correspond to anything I've ever wanted
to do, even in languages that already have multiline strings. At least,
could we have an overload that just takes the starting marker, for the
overwhelmingly commoner case where you only want to strip at the start?

On Tue, 10 Apr 2018 at 13:50, Brian Goetz  wrote:




(now stripIndent)

I've accumulated a few questions/comments on this.

1. When choosing the amount to trim, it ought to ignore blank lines and
only-whitespace lines, right?

Seems right.

2. Is it really appropriate to automatically remove trailing whitespace?

I'm not sure about this either.  The reason that RSLs will have "extra"
whitespace that needs to be stripped is that we want to indent the RSL
snippet relative to the Java code (and as you point out, the IDE may do
that automatically for us.)  But if there's trailing whitespace, its
because the user put it there, and who is it hurting?  It might be
significant.

3. If the input contains *any* tab characters at all (except any that

are

part of the trailing whitespace), then this method cannot know that it
isn't jumbling the end result, and maybe it should just throw.

I think there's a middle ground, where it strips any common whitespace
prefix.  So if every line starts with tab-tab-space-space, it can safely
strip this.

5. If we do end up in a world where we have to call this for almost

every

one of our tens of thousands of multi-line RSLs... is it strange that I
feel like I would prefer it was static? It seems like it would look a

lot

more normal that way visually. Ugh...

I think this is likely to vary subjectively a lot.  Some people like
that the instance method is mostly out of the way; others like the
up-front shouting of the static method.
The reason we can't have both is then we can't resolve the method
reference String::strip as a Function, which seems a
useful thing to do.

On top of *that*, I have no idea what "right markers" are good for, nor
what customizing the marker choice is good for (other than creating more
needless variation between different pieces of code).


String asciiArtFTW =
`
 

Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater

[Xueming Shen]

Hi David,

The CSR has been approved
https://bugs.openjdk.java.net/browse/JDK-8200527

API docs have been updated slightly based on the review suggestion.

(1) added some words in the class spec for both Inflater and Deflater.

|*
 * This class deflates sequences of bytes into ZLIB compressed data format.
 * The input byte sequence is provided in either byte array or byte buffer,
 * via one of the {@code setInput()} methods. The output byte sequence is
 * written to the output byte array or byte buffer passed to the
 * {@code deflate()} methods.
 *|

|*
 * This class inflates sequences of ZLIB compressed bytes. The input byte
 * sequence is provided in either byte array or byte buffer, via one of the
 * {@code setInput()} methods. The output byte sequence is written to the
 * output byte array or byte buffer passed to the {@code deflate()} methods.
 *|


(2) adjusted the workding a little for those setInput() methods

|  *
 * One of the {@code setInput()} methods should be called whenever
 * {@code needsInput()} returns true indicating that more input data
 * is required.
 *|


Two issues have been noticed when running tier1/2/3 tests

(1) there is a error at ln#Inflater.c#243, the "input" is being released 
instead of "output"

http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev.00/src/java.base/share/native/libzip/Inflater.c.sdiff.html


which triggered crash for some tests. fixed.

(2) sun/nio/ch/TestMaxCachedBufferSize.java failed "because of" the 
"defaultBuf"
 uses direct ByteBuffer. This is probably the issue of the test but 
I simply update
 the "defaultBuf" to be the heap buffer/0, instead of touch the 
failed test case.
 I don't have problem if you prefer to "fix" the test and keep the 
"defaultBuf" as

 direct buffer instead.

   // static final ByteBuffer defaultBuf = ByteBuffer.allocateDirect(0);
static final ByteBuffer defaultBuf = ByteBuffer.allocate(0);

(3) I also updated test/jdk/java/util/zip/DeInflate.java with more tests 
for the new APIs.

 More tests might be desired though.

The latest webrev is at

http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/

Thanks,
Sherman

RFR 8197418: Move java/util/RandomAccess/ tests into OpenJDK

[Chris Yin]

Please review the change to move java/util/RandomAccess/Basic.java test into 
OpenJDK, it increases code coverage of open jdk_util test group per JCov run, 
the main improvement is on java.util.Collections with %method up 1% and %block 
up 2%, thanks

bug: https://bugs.openjdk.java.net/browse/JDK-8197418 

webrev: http://cr.openjdk.java.net/~xyin/8197418/webrev.00/ 


Regards,
Chris