On Nov 15, 4:53 am, Attila Szegedi <[EMAIL PROTECTED]> wrote:
> Hi folks,
>
> I run FindBugs on Clojure source code, and there are few things it
> uncovered. I'd be happy to fix these and submit patches (after I
> submitted a contributor agreement), except if someone already a
> contributor wants to tackle these instead (they're easy to fix for the
> most part).
>
> 1. withMeta() in CachedSeq, LazilyPersistedVector, and LazyCons isn't
> synchronized, and it looks like a potential for race condition. I've
> looked into some usages of withMeta() to make sure I'm not crying wolf
> in vain (i.e. there's a guarantee of synchronization higher up the
> call chain), but it looks like there isn't, so I'm fairly certain this
> is a real problem.
>
> 2. Keyword and Ref define compareTo, but don't redefine equals (and
> hashCode) to be consistent with it. It ain't necessarily a problem if
> you know what you're doing, but since they're public it's usually a
> good rule of thumb to have equals consistent with compareTo (and then
> hashCode consistent with equals).
>
> 3. There are lots of usages of "new Integer(...)", "new Short(...)",
> "new Long(...)" etc. Now, in Java 5 and above, "Integer.valueOf(...)"
> and similar methods for all primitive wrapper types are the preferred
> idioms as the valueOf() methods return canonical instances for certain
> value range (usually -128..127) - these are used behind the scenes for
> Java autoboxing as well.
>
> There are a lot of other FindBugs warnings as well, but I won't go
> into them as any contributor/committer can run FindBugs on their own
> and review them. These three however I think desire emphasis.
>
Welcome Attila,
I've run findbugs on Clojure before and cleaned up a few things. These
that you mentioned, however, stand as a good example of such an
analyzer not knowing enough, and I count as spurious, if well-
intended.
As mentioned by Phil, the refs are still correct. The UUID is some
unfinished work on ref persistence (of the db kind). Don't look behind
the curtain :)
As far as the synchronization, let's take LazyCons. _first and _rest
are normally accessed under a lock. The semantics and behavior of the
class is such that these fields make a once-and-once-only transition
into a final state. Those transitions happen under locks, in first()
and rest(), and rest() calls first().
So, in the withMeta call, the call to rest() ensures that the
subsequent use of _first and _rest will see the final values. I don't
expect findbugs to understand that, but it's certainly correct.
CachedSeq is similar. In LazilyPersistentVector the v field is an
optimization cache and can be freely recreated.
As far as valueOf, Cliff Click at the JVM Languages Summit had some
interesting details about how those caches can thwart optimization in
the presence of escape analysis. Instead of having a simple path, they
cause a branch into array lookup code. Without such a branch an
optimizer could get rid of the boxed object entirely in some
circumstances. It left me skeptical of the universal value of valueOf.
Rich
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Clojure" group.
To post to this group, send email to clojure@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/clojure?hl=en
-~----------~----~----~----~------~----~------~--~---
