[CLJ-787] transient blows up when passed a vector created by subvec Created: 03/May/11 Updated: 08/Nov/13
|Fix Version/s:||Release 1.6|
|Patch:||Code and Test|
Subvectors created with subvec from a PersistentVector cannot be made transient:
Cause: APersistentVector$SubVector does not implement IEditableCollection
Approach: Create a TransientSubVector based on an underlying TransientVector.
|Comment by Stuart Sierra [ 31/May/11 9:28 AM ]|
Confirmed. APersistentVector$SubVector does not implement IEditableCollection.
The current implementation of TransientVector depends on implementation details of PersistentVector, so it is not a trivial fix. The simplest fix might be to implement IEditableCollection.asTransient in SubVector by creating a new PersistentVector, but I do not know the performance implications.
|Comment by Gary Fredericks [ 25/May/13 8:11 PM ]|
We could get the same performance characteristics as SubVector by creating a TransientSubVector based on an underlying TransientVector, right?
Preparing a patch to that effect.
|Comment by Gary Fredericks [ 25/May/13 10:58 PM ]|
Text from the commit msg:
Made two assumptions:
|Comment by Alex Miller [ 11/Oct/13 4:17 PM ]|
I think there are some assumptions being made in this patch about the class structure here that do not hold. The structure is, admittedly, quite twisty.
A counter-example that highlights one of a few subtypes of APersistentVector that are not PersistentVector (like MapEntry):
PersistentVector.SubVector expects to work on anything that implements IPersistentVector. Note that this includes concrete types such as MapEntry and LazilyPersistentVector, but could also be any user-implemented type IPersistentVector type too. TransientSubVector is making the assumption that the IPersistentVector in a SubVector question is also an IEditableCollection (that can be converted to be transient). Note that while PersistentVector implements TransientVector (and IEditableCollection), APersistentVector does not. To really implement this in tandem with SubVector, I think you would need to guarantee that IPersistentVector extended IEditableCollection and I don't think that's something we want to do.
I don't see an easy solution. Any time I see all these modifiers (Transient, Sub, etc) being created in different combinations, it is a clear sign that independent kinds of functionality are being remixed into single inheritance OO trees. You can see the same thing in most collection libraries (even Java's - need a ConcurrentIdentitySortedMap? too bad!).
Needs more thought.
|Comment by Andy Fingerhut [ 08/Nov/13 10:17 AM ]|
Patch CLJ-787-p1.patch no longer applies cleanly to latest master, but it is only because of some new tests added to the transients.clj file since the patch was created, so it is trivial to update in that sense. Not updating it now due to other more significant issues with the patch described above.