-
Notifications
You must be signed in to change notification settings - Fork 381
Java 12-17API additions #10106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Java 12-17API additions #10106
Conversation
42c7d0a
to
ec22afe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on the draft - some quick feedback
.../test-super/com/google/gwt/dev/jjs/super/com/google/gwt/emultest/java12/lang/StringTest.java
Outdated
Show resolved
Hide resolved
user/test/com/google/gwt/emultest/java12/util/stream/CollectorsTest.java
Outdated
Show resolved
Hide resolved
.../com/google/gwt/dev/jjs/super/com/google/gwt/emultest/java12/util/stream/CollectorsTest.java
Outdated
Show resolved
Hide resolved
user/super/com/google/gwt/emul/java/util/stream/Collectors.java
Outdated
Show resolved
Hide resolved
.../com/google/gwt/dev/jjs/super/com/google/gwt/emultest/java12/util/stream/CollectorsTest.java
Outdated
Show resolved
Hide resolved
assertEquals(3, hideFromCompiler("foo").transform(String::length)); | ||
} | ||
|
||
public void testIndent() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also include a set of these tests that does not do a hideFromCompiler, so we can make sure that if the compiler is able to constant-fold these method calls, that it does so correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not implement any tests without hideFromCompiler
yet since I wasn't sure which methods need them. It seems that tests either hide all inputs from compiler (like the ones from https://github.com/gwtproject/gwt/pull/9975/files#diff-79013db0b72ddfb9522327d93e6c5804f44c160025590e971ddfc7fd4eb7c045R26 ) or hide nothing (https://github.com/zbynek/gwt/blob/f0c1486a46018aec9de93458136cfc161f569e81/user/test/com/google/gwt/emultest/java/lang/StringTest.java#L542 ). Also I'm not sure if it wouldn't be better to test the constant-folding optimization and verify that these are actually folded rather than make assertions where we don't know if we're testing folding or emulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using hideFromCompiler ensures that the emulated sources provided can be called and function the way that the test expects. Passing a constant and allowing constant folding means that the operation will be performed by the compiler itself, using the JRE's own implementation, which does two things for us: we know that constant folding doesn't somehow break with this call, and that the JRE implementation doesn't behave in a way that fails the test.
This isn't a guarantee - the constant folding mechanism is pretty specific (I've been meaning to file a bug about that) and won't always kick in, but it does let us get a little more coverage.
077dfea
to
55d86e0
Compare
033deb4
to
b6adb5d
Compare
0257e71
to
b8996b5
Compare
ec661eb
to
a8e31dc
Compare
user/super/com/google/gwt/emul/java/util/stream/LongStream.java
Outdated
Show resolved
Hide resolved
...m/google/gwt/emultest/super/com/google/gwt/emultest/java17/util/stream/DoubleStreamTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Colin Alworth <[email protected]>
https://github.com/zbynek/gwt/actions/runs/16452377759 is 🟢 , @niloc132 thanks for the feedback |
...test-super/com/google/gwt/emultest/super/com/google/gwt/emultest/java17/lang/StringTest.java
Show resolved
Hide resolved
@@ -488,10 +491,6 @@ public String intern() { | |||
return checkNotNull(this); | |||
} | |||
|
|||
public boolean isEmpty() { | |||
return length() == 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
far-off future nit: this is going to end up being inlined as something like
<expr>.length==0
, and promoting this to the supertype (which, i think i suggested) will mean we keep getting that. But, it would be smaller to just implement as <expr>==""
in the cases where we know we have a java.lang.String, since java String is final+immutable, and js string can be ==
/===
compared. I think we could also live with !<expr>.length
, but that's still longer regardless of == or === usage.
@@ -811,6 +814,131 @@ private int getTrailingWhitespaceLength() { | |||
return length; | |||
} | |||
|
|||
public String indent(int spaces) { | |||
if (spaces == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkstyle ignores supersource, for better or worse.
if (spaces == 0) { | |
if (spaces == 0) { |
EDIT: actually this is wrong, javadoc specifies that line endings are rewritten
dev/core/src/com/google/gwt/dev/javac/testing/impl/JavaResourceBase.java
Show resolved
Hide resolved
|
||
public String stripIndent() { | ||
if (isEmpty()) { | ||
return ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically this could also be return this;
, since the "isEmpty" check is "are you the empty string". Might result in making the code bigger though, but probably will be smaller - String is final so all methods are made static and this
is an argument (so will be rewritten to argument a
or something) - one char instead of two.
I'm not 100% confident in that analysis, so maybe don't change this just from my guesswork, just trying to think through what JS we'll get.
assertEquals("x\ny\n", hideFromCompiler(" x\n y").indent(-2)); | ||
assertEquals("x\ny\n", hideFromCompiler(" x\r\n y").indent(-2)); | ||
assertEquals("x\ny\n", hideFromCompiler(" x\r y").indent(-2)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/String.html#indent(int), we should have a few more tests:
- validate that
\t
is removed as if it was a single space - validate that indent(0) changes line endings to
\n
- this will fail - generally validate that
\r\n
is rewritten to\n
(edit: this is covered for positive and negative already I see) - validate that negative indent with fewer spaces just removes what spaces are available - and that indent with too many spaces leaves extra spaces.
- validate that the empty string is handled correctly, and other strings with no line break - I think this will fail
- validate trailing/leading linebreak in the string
} else { | ||
indentedLines = lines().map( | ||
line -> line.substring(Math.min(-spaces, line.getLeadingWhitespaceLength()))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a little more code, this could be a little faster in the spaces == 0
case, just assign indentedLines = lines()
with no calls to getLeadingWhitespaceLength(), bounds checks, or min() tests for each line. I think we would rather the code you have now though, as that seems a little silly to do, but just pointing it out.
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/String.html#indent(int)
It would be good to have tests that validate this too, added a note below for this.
Co-authored-by: Colin Alworth <[email protected]>
Fixes #9872
Fixes #10091
Fixes #9991