On Fri, 2 May 2025 17:15:29 GMT, Hendrik Schick <d...@openjdk.org> wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8356049
>
> src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java
> line 101:
>
>> 99: clip.init(stream);
>> 100: } catch (final Exception e) {
>> 101: // AudioClip will be no-op if some exception will occurred
>
> Suggestion:
>
> // AudioClip will be no-op if some exception will occur
>
> or 'has occurred' (?)
I'm just going to delete the comment here as it is a bit out of place.
The more interesting question is whether IOException should be thrown or
swallowed like all other exceptions.
The current code throws it (clearly) and so the app has to handle it.
But if we swallow other exceptions ... ?
> src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java
> line 114:
>
>> 112: clip.init(uc.getInputStream());
>> 113: } catch (final Exception ignored) {
>> 114: // Playing the clip will be a no-op if an exception occured
>> in inititialization.
>
> Suggestion:
>
> // Playing the clip will be a no-op if an exception occurred in
> initialization.
No plans to fix typos comments for code I'm not touching and expect to delete
in the near future
> src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java
> line 124:
>
>> 122: clip.init(url.openStream());
>> 123: } catch (final Exception ignored) {
>> 124: // Playing the clip will be a no-op if an exception
>> occurred in inititialization.
>
> Suggestion:
>
> // Playing the clip will be a no-op if an exception occurred in
> initialization.
No plans to fix typos comments for code I'm not touching and expect to delete
in the near future
> src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java
> line 294:
>
>> 292: public synchronized void update(LineEvent event) {
>> 293: if (clip != null) {
>> 294: if (clip == event.getSource()) {
>
> merge the two ifs to one if?
I could .. but this way it is clearer that clip may actually be null ..
> test/jdk/javax/sound/SoundClip/SoundClipTest.java line 40:
>
>> 38:
>> 39: public static void main(String[] args) throws Exception {
>> 40:
>
> Suggestion:
I like blank lines there
> test/jdk/javax/sound/SoundClip/SoundClipTest.java line 100:
>
>> 98: }
>> 99: } catch (Exception e) {
>> 100: System.err.println("Exception occured: "+e);
>
> Suggestion:
>
> System.err.println("Exception occurred: " + e);
I copied that from another test.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072221321
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072221421
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072221490
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072222130
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072222289
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072222456
