Skip to content
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

Restructuring for File.read #3390

Merged
merged 35 commits into from
Apr 19, 2022
Merged

Restructuring for File.read #3390

merged 35 commits into from
Apr 19, 2022

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Apr 11, 2022

Pull Request Description

  • Added Encoding type
  • Added Text.bytes, Text.from_bytes with Encoding support
  • Renamed File.read to File.read_text
  • Renamed File.write to File.write_text
  • Added Encoding support to File.read_text and File.write_text
  • Added warnings to invalid encodings

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

@jdunkerley jdunkerley force-pushed the wip/jd/file-read-181823873 branch 2 times, most recently from 6dfa047 to 333eee3 Compare April 14, 2022 11:16
@jdunkerley jdunkerley marked this pull request as ready for review April 14, 2022 16:40
@jdunkerley jdunkerley requested a review from 4e6 as a code owner April 14, 2022 16:40
@jdunkerley jdunkerley requested a review from hubertp April 15, 2022 06:08
@@ -153,14 +238,67 @@ public static String from_codepoints(int[] codepoints) {
return new String(codepoints, 0, codepoints.length);
}

private static CharBuffer resize(CharBuffer old) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DRY?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

felt the generic solution wasn't very clean:

  private static <T extends Buffer> T resize(T old, IntFunction<T> allocate, BiConsumer<T, T> put) {
    int n = old.capacity();
    int new_n = 2*n + 1;
    T o = allocate.apply(new_n);
    old.flip();
    put.accept(o, old);
    return o;
  }

as Buffer doesn't have put or allocate. But happy to switch to generic if you think better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually argue for having the two specializations.
Maybe it is premature optimization, but feels like having ByteBuffer or CharBuffer directly can allow the JVM to statically resolve the methods to be used whereas having the closures it may not realise this is possible, at least that quickly. And it may be important because encoding is quite a primitive operation, which can work with quite big amounts of data and so performance is valuable here.

But if @hubertp you think it shouldn't matter much (I don't have enough JVM experience to judge if the overhead of closures here is noticeable or not), then I agree that a generic solution is more elegant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't think it will have any performance overhead as just method references being passed - does make it slightly cleaner to only have the method once.

The Buffer class is ancient so not thought about for this stuff!

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but some changes are needed.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great now, as long as tests go through should be good to go :)

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Apr 19, 2022
@mergify mergify bot merged commit 5a6b674 into develop Apr 19, 2022
@mergify mergify bot deleted the wip/jd/file-read-181823873 branch April 19, 2022 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants