Provide an implicit for AutoCloseable instead of Closeable #80
+5
−5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cchantep
reviewed
Aug 18, 2019
| @@ -44,4 +44,4 @@ Type classes in scala are encoded using implicit parameters. Due to the mechan | |||
|
|
|||
| These two implicits allow users of the library to make use of any resource class that defines a close or dispose method. The downside is that by using structural types, the close and dispose method will be invoked via reflection. | |||
|
|
|||
| The scala-arm library also provides default implicits for common resources used from the Scala library or the Java SDK. In particular, anything that extends `java.io.Closeable` will *not* use reflection when closing the resource. This type class is given slightly higher priority so it will be preferred to the reflective version when resolving implicits. | |||
There was a problem hiding this comment.
For me, that would be a breaking change to right the stronger AutoCloseable to be required
There was a problem hiding this comment.
AutoCloseable is weaker though? All Closeables are AutoCloseables?
|
Right I always find the JSE naming misleading about that. |
cchantep
approved these changes
Aug 19, 2019
|
What is the etiquette here? Should I merge now or let one of the maintainers do the honors? |
|
(And thanks for the quick approval!) |
|
Can merge once approved and passing CI |
|
Oh I see, I can't merge anyways. I'll let one of you do it, thanks again! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now, Resources can be implicitly derived AutoCloseables, but (surprisingly) only through reflection, which incurs an enormous performance penalty. I'm not really sure why -- java.lang.AutoCloseable is the prescribed interface for ARM in java and has been for quite some time. I'm guessing there is some reason for keeping things this way since this change is so trivial, but I thought I'd put it up to start discussion.
Fixes #66.
Thanks!