-
Notifications
You must be signed in to change notification settings - Fork 39
Add MustCall annotations to resource-owning JDBC, MIDI, and audio interfaces #254
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: master
Are you sure you want to change the base?
Changes from all commits
473fc2e
e278dec
b7542c4
62b0944
bb62832
eb6ecb7
050ad69
edd8992
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,9 @@ | |
|
|
||
| package javax.sound.sampled; | ||
|
|
||
| import org.checkerframework.checker.mustcall.qual.InheritableMustCall; | ||
| import org.checkerframework.framework.qual.AnnotatedFor; | ||
|
|
||
| /** | ||
| * The {@code Line} interface represents a mono or multi-channel audio feed. A | ||
| * line is an element of the digital audio "pipeline," such as a mixer, an input | ||
|
|
@@ -66,6 +69,8 @@ | |
| * @see LineEvent | ||
| * @since 1.3 | ||
| */ | ||
| @AnnotatedFor({"mustcall"}) | ||
| @InheritableMustCall("close") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is a
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It’s the latter: a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit tricky. I think ideally, we'd annotate any relevant methods that create an unopened
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need something like this for this instance custom one maybe?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @msridhar If that approach seems reasonable, I’m happy to prototype it and share a draft PR for feedback before applying it to
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry @parameshn I've been very busy, hoping to get back to this next week
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, thanks for the update! Looking forward to your thoughts. |
||
| public interface Line extends AutoCloseable { | ||
|
|
||
| /** | ||
|
|
||
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.
Based on the Javadoc, I think we may need
@NotOwningannotations on methods likeMidiSystem#getReceiverandMidiSystem#getTransmitter, to indicate that theMidiDevicereturned by those APIs does not need to be explicitly closed. Do you agree?Uh oh!
There was an error while loading. Please reload this page.
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 my understanding, when
MidiSystem.getReceiver()/getTransmitter()are used, the underlyingMidiDeviceis opened implicitly by the system and closed implicitly when the returnedReceiver / Transmitteris closed (assuming no other Receivers/Transmitters remain open). As a result, the application does not own theMidiDevice; its responsibility is only to close the returnedReceiver / Transmitter. Since theMidiDeviceis not exposed by these APIs, there isn’t really a place to annotate it with@NotOwningat the API level.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 think I misunderstood what was going on here. We don't need
@NotOwningannotations on these APIs.