Spotbugs #1076
Spotbugs #1076
Conversation
| @@ -26,6 +27,7 @@ | |||
| /** | |||
| * @author Boris Grozev | |||
| */ | |||
| @SuppressFBWarnings("SE_BAD_FIELD") | |||
There was a problem hiding this comment.
Reading the description of this warning, I wonder if it's one we should disable overall?
| @@ -88,7 +88,7 @@ private static void check(Conference conference) | |||
| throw new RuntimeException(ioe); | |||
| } | |||
|
|
|||
| endpoints.add(endpoint); | |||
| //endpoints.add(endpoint); | |||
There was a problem hiding this comment.
There's a block of code below which uses endpoints, but it's commented out.
There was a problem hiding this comment.
Ah, ok...I expanded the file and see now.
| @SuppressWarnings("unchecked") | ||
| @SuppressFBWarnings( | ||
| value = "IS2_INCONSISTENT_SYNC", | ||
| justification = "We intentionally avoid synchronizing while reading" + |
There was a problem hiding this comment.
Reading through this class, I find the synchronization we're doing a bit confusing (I probably haven't parsed it fully) but I wonder if it's worth a look.
There was a problem hiding this comment.
I don't know whether the syncronization in the rest of the class makes sense, but for getDebugState my intention is as described: read the int fields without aquiring any locks to reduce the risk of a deadlock (since this runs in a completely different context)
| @@ -96,6 +99,10 @@ | |||
| * payload type (could be VP9, could be H264, could be VP8) so it has to be | |||
| * created on packet arrival. | |||
| */ | |||
| @SuppressFBWarnings( | |||
There was a problem hiding this comment.
Instead of this, I wonder if we should just get rid of the synchronized on getContext (the comments there even state it isn't necessary now).
| @@ -885,9 +897,6 @@ public final Videobridge getVideobridge() | |||
| */ | |||
| public boolean isExpired() | |||
| { | |||
| // Conference starts with expired equal to false and the only assignment | |||
| // to expired is to set it to true so there is no need to synchronize | |||
| // the reading of expired. | |||
| return expired; | |||
There was a problem hiding this comment.
I'd keep this explanation, which is better than just "is deemed safe" imo.
| @@ -484,12 +478,14 @@ private VP8FrameProjection createInLayerProjection(@NotNull VP8Frame frame, | |||
| } | |||
| else | |||
| { | |||
| do { | |||
| do | |||
| { | |||
| f2 = prevFrame(f1); | |||
There was a problem hiding this comment.
This will probably conflict when you rebase on master, so you might want to do that sooner rather than later.
| @@ -145,7 +145,7 @@ static MediaType readMediaType(byte[] buf, int off, int len) | |||
| case OCTO_MEDIA_TYPE_DATA: | |||
| return MediaType.DATA; | |||
| default: | |||
| return null; | |||
| throw new IllegalArgumentException("Invalid media type value" + mediaType); | |||
I fixed or supressed the obvious warnings, but there are 9 warning left that are valid and require some thought. That's why I've not added it to the
verifyphase in maven yet.