Why is CachedValue not copyable?

What is the rationale for CachedValue not being copyable?

From what I gather, a CachedValue can be used alongside a stored ValueTree to provide easy getter and setter wrappers around a property, this way providing some type safety and abstraction over the raw property.

However, storing such CachedValues as data members next to the ValueTree data member makes the containing class non-copyable (by default), because CachedValue is explicitly made non-copyable.

If I understand correctly, it’s valid anyway to create multiple CachedValue instances which wrap the same property (which could happen with multiple listeners to the same ValueTree). So why not just make CachedValue's copy-able, allowing creation of a new wrapper around the same property?

Thanks,
Dan

5 Likes

following… :wink:

FYI, you can just set to be notified of any replies on a post should you wish:


:wink:

3 Likes

I’m interested in this too. I have some lightweight wrappers around ValueTrees and it would be nice to be able to pass them around and make copies.

1 Like

CachedValue is “weak-referenceable”

which also makes it non-copiable.

I don’t know how many people are using WeakReference<CachedValue>, but it would be a pretty important breaking change nonetheless.

If you’re fine with forking JUCE, you can apply a diff similar to this (:warning: I’ve added a test, but it doesn’t guarantee that it works in all cases):

diff --git a/modules/juce_data_structures/values/juce_CachedValue.cpp b/modules/juce_data_structures/values/juce_CachedValue.cpp
index dbf070b4b..2c34fd3d9 100644
--- a/modules/juce_data_structures/values/juce_CachedValue.cpp
+++ b/modules/juce_data_structures/values/juce_CachedValue.cpp
@@ -146,6 +146,22 @@ public:
 
             expect (t["testkey"] == var());
         }
+
+        beginTest ("copy CachedValue object");
+        {
+            ValueTree t ("root");
+            t.setProperty ("testkey", "oldvalue", nullptr);
+
+            CachedValue<String> cv1 (t, "testkey", nullptr, "defaultvalue");
+            expect (cv1 == "oldvalue");
+
+            CachedValue<String> cv2 (cv1);
+            expect (cv2 == "oldvalue");
+
+            t.setProperty ("testkey", "newvalue", nullptr);
+            expect (cv1 == "newvalue");
+            expect (cv2 == "newvalue");
+        }
     }
 };
 
diff --git a/modules/juce_data_structures/values/juce_CachedValue.h b/modules/juce_data_structures/values/juce_CachedValue.h
index 6236e5065..0281dcf74 100644
--- a/modules/juce_data_structures/values/juce_CachedValue.h
+++ b/modules/juce_data_structures/values/juce_CachedValue.h
@@ -92,6 +92,9 @@ public:
     CachedValue (ValueTree& tree, const Identifier& propertyID,
                  UndoManager* undoManager, const Type& defaultToUse);
 
+    /** Copy constructor. */
+    CachedValue (const CachedValue& other);
+
     //==============================================================================
     /** Returns the current value of the property. If the property does not exist,
         returns the fallback default value.
@@ -197,10 +200,6 @@ private:
     Type getTypedValue() const;
 
     void valueTreePropertyChanged (ValueTree& changedTree, const Identifier& changedProperty) override;
-
-    //==============================================================================
-    JUCE_DECLARE_WEAK_REFERENCEABLE (CachedValue)
-    JUCE_DECLARE_NON_COPYABLE (CachedValue)
 };
 
 
@@ -224,6 +223,14 @@ inline CachedValue<Type>::CachedValue (ValueTree& v, const Identifier& i, UndoMa
     targetTree.addListener (this);
 }
 
+template <typename Type>
+inline CachedValue<Type>::CachedValue (const CachedValue& other)
+    : targetTree (other.targetTree), targetProperty (other.targetProperty), undoManager (other.undoManager),
+      defaultValue (other.defaultValue), cachedValue (other.cachedValue)
+{
+    targetTree.addListener (this);
+}
+
 template <typename Type>
 inline Value CachedValue<Type>::getPropertyAsValue()
 {
3 Likes

Looks like it’s been made WEAK_REFERENCEABLE in commit c29eea4. Before that, it was only directly declared non-copyable (which we could change if we agreed it made sense).

Unfortunately, the commit doesn’t say the reasoning for making it weak referenceable - @ed95, as the author of that commit, do you happen to recall the rationale?

Thanks,
Dan

1 Like

Thanks. That post does show the rationale for JUCE_DECLARE_WEAK_REFERENCEABLE, although it seems like an implementation detail (probably tweakable if we want to avoid making CachedValue non-copyable) to make the proposed CachedValueSource implementation better - which was never implemented (or at least I couldn’t find it in the JUCE code).

@dave96, is it by-design that CachedValue isn’t copyable? (currently both implicitly because of being weak-referenceable, and explicitly because it is declared non-copyable). If so, what is the rationale? As I wrote earlier, this makes it harder to copy around objects of classes which wrap a ValueTree and CachedValues into it (such as Clip from your presentation on ValueTrees).

Thanks!
Dan

1 Like

Tbh I can’t remember if it was by design. We wrote that class many years ago now.

Looking at our code, I don’t actually see any uses of WeakReference<CachedValue>, but that’s probably because I forgot to update them since Ed added it. I can see the use case for it.

In terms of practicality, very few of our internal Tracktion Engine classes (e.g. Clip, Track, Plugin etc.) are copyable because that operation isn’t well defined. What would a copy of a clip look like? Is that a new clip? But referring to the same underlying data?
We deliberately disable those classes from being copyable and use a dedicated Clipboard class to copy/duplicate objects as it’s often more complicated than you think (e.g. what do you do with automation data when copying/moving clips).

Is there a concrete use-case for making it copyable? The one stated in the original post seems more of an “ideal” than “this is a problem for me”…
I only say this because there is a concrete use case for making it non-copyable.

Wouldn’t the alternative be to simply provide a copy constructor though? Presumably that wouldn’t copy the weak-reference but would create a new CachedValue as it you simply passed the same constructor parameters?
Would that be semantically correct?

3 Likes

Good point. I assumed that JUCE_DECLARE_WEAK_REFERENCEABLE was in the way because it makes CachedValue implicitly non-copyable, but we can have both JUCE_DECLARE_WEAK_REFERENCEABLE and a copy-constructor. Here is the updated diff (without the unit test):


diff --git a/modules/juce_data_structures/values/juce_CachedValue.h b/modules/juce_data_structures/values/juce_CachedValue.h
index 6236e5065..2d25e53b5 100644
--- a/modules/juce_data_structures/values/juce_CachedValue.h
+++ b/modules/juce_data_structures/values/juce_CachedValue.h
@@ -92,6 +92,9 @@ public:
     CachedValue (ValueTree& tree, const Identifier& propertyID,
                  UndoManager* undoManager, const Type& defaultToUse);
 
+    /** Copy constructor. */
+    CachedValue (const CachedValue& other);
+
     //==============================================================================
     /** Returns the current value of the property. If the property does not exist,
         returns the fallback default value.
@@ -200,7 +203,6 @@ private:
 
     //==============================================================================
     JUCE_DECLARE_WEAK_REFERENCEABLE (CachedValue)
-    JUCE_DECLARE_NON_COPYABLE (CachedValue)
 };
 
 
@@ -224,6 +226,14 @@ inline CachedValue<Type>::CachedValue (ValueTree& v, const Identifier& i, UndoMa
     targetTree.addListener (this);
 }
 
+template <typename Type>
+inline CachedValue<Type>::CachedValue (const CachedValue& other)
+    : targetTree (other.targetTree), targetProperty (other.targetProperty), undoManager (other.undoManager),
+      defaultValue (other.defaultValue), cachedValue (other.cachedValue)
+{
+    targetTree.addListener (this);
+}
+
 template <typename Type>
 inline Value CachedValue<Type>::getPropertyAsValue()
 {
1 Like

I ran into this in my code when I refactored it to not use a raw ValueTree, but a type-safe wrapper around it which contains the tree as well as CachedValue for getting and setting properties. We had a couple of places where we made a copy of the ValueTree, which broke under that refactor.

I think the general use case would be if the same property would be accessible and/or settable from multiple controllers. It seems to me to make sense to copy the wrapping class just like I would the ValueTree (creating a reference to it).

It makes sense to me that CachedValue would be copyable because I can instantiate multiple CachedValues which refer to the property in a tree. The semantics would be referring to the same property, just as I would get if I just made two instances myself and set them up to refer to the same property.

But the class originally has JUCE_DECLARE_NON_COPYABLE so I there might be a rationale for it. By “there is a concrete use case for making it non-copyable” do you just mean it being weak-referenceable (which as you suggested can be overcome) or something else?

Yes, I guess I worded that badly. I meant there is a concrete case for making it weak-referenceable.

It seems like adding the copy constructor would be the most sensible approach (although in this case I’d advocate for the rule of 5 and also add copy-asignment operators and move construct/assign operators, assuming the weak-ref doesn’t make these impossible?).

1 Like

I’ll toss in a vote for adding a copy constructor to CachedValue<>. My use case is similar to danradix’s. I’ve got some wrappers around ValueTrees, which contain CachedValue<>s. In a few situations I’d like to be able to copy them, use them as return values from functions, etc. In these cases they are not responsible for owning the underlying ValueTree so when they are copied, they make a copy of the ValueTree internally (which ultimately wraps the same dynamic object).

So, I’d expect that CachedValue<> would be copy-constructable and would referTo a copy of the other’s ValueTree. Otherwise it’s a bit of a pain to add copy constructors to my wrapper classes, because the compiler can’t provide a default.

Of course copy-assignment would be great to have as well.

Hmm. When you have things like CachedValue where there are listeners involved, I’ve always been a bit hesitant about making it easy to copy or assign them.

CachedValue for example has an assignment operator that takes the raw value. So right now if you had CachedValue<int> v1, v2, and you do v1 = v2, it’d copy the integer value across. If we added copy/assign to the class, then the same line of code would do something very different - it’d seem to end up with the same value, but would have detatched v1 from its original ValueTree, lost all the listeners, and attached to some other tree.

Good point. Maybe just a copy constructor is safest for now then.

Hey, can you show a snippet of how this is faster/quicker than accessing the tree properties directly?

Performance is less of what I was trying to achieve this. I was going for type safety. See @dave96’s presentation at https://www.youtube.com/watch?v=3IaMjH5lBEY

I suspect that could actually lead to more confusion than today :frowning: https://coliru.stacked-crooked.com/a/4a0f831f9d9cd768

I didn’t consider Jules’s example. I guess since that’s the behavior of the assignment operator today for CachedValues, it would be problematic to change it (e.g. by making the cast operator to Type explicit) since it could change the behavior of existing code silently (once an assignment operator is also implemented).

I understand the assignment issue that Jules describes, but isn’t that issue already present with the ValueTrees?

ValueTree a, b;
a = b; // ?

However, when I write wrappers for ValueTree’s, I never use assignment (e.g assigning a clip to another clip) but I do copy them around, because from my opinion copying a wrapper should behave similar to copying a ValueTree: keeping a reference to the same underlying object.