Make SkFunction copyable so it can go in containers.

src/core/SkFunction.h

Index: src/core/SkFunction.h
diff --git a/src/core/SkFunction.h b/src/core/SkFunction.h
index 43438dd9e29970f1ea619cc3e6067fcf4abc11a2..d6ebd11b43f6abcfbd8554050284a7feded50e95 100644
--- a/src/core/SkFunction.h
+++ b/src/core/SkFunction.h
@@ -8,94 +8,68 @@
 #ifndef SkFunction_DEFINED
 #define SkFunction_DEFINED
 
-// TODO: document
+// TODO: document, more pervasive move support in constructors, small-Fn optimization
 
+#include "SkTemplates.h"
 #include "SkTypes.h"
-#include "SkTLogic.h"
 
 template <typename> class SkFunction;
 
 template <typename R, typename... Args>
-class SkFunction<R(Args...)> : SkNoncopyable {
+class SkFunction<R(Args...)> {
 public:
-    SkFunction(R (*fn)(Args...)) : fVTable(GetFunctionPointerVTable()) {
-        // We've been passed a function pointer.  We'll just store it.
-        fFunction = reinterpret_cast<void*>(fn);
-    }
+    SkFunction() {}
 
     template <typename Fn>
-    SkFunction(Fn fn, SK_WHEN_C((sizeof(Fn) > sizeof(void*)), void*) = nullptr)
-            : fVTable(GetOutlineVTable<Fn>()) {
-        // We've got a functor larger than a pointer.  We've go to copy it onto the heap.
-        fFunction = SkNEW_ARGS(Fn, (Forward(fn)));
-    }
+    SkFunction(const Fn& fn) : fFunction(SkNEW_ARGS(LambdaImpl<Fn>, (fn))) {}

[bungeman-skia, 2015/05/06 14:16:31]

It's been a while now, but not having the small function optimization makes
these rather less useful from a performance standpoint doesn't it? In some cases
it doesn't matter, but in some places it seemed like it did.

[mtklein, 2015/05/06 14:22:10]

Yes, for sure.  But, not being able to copy SkFunctions makes them even less
useful.

 
-    template <typename Fn>
-    SkFunction(Fn fn, SK_WHEN_C((sizeof(Fn) <= sizeof(void*)), void*) = nullptr)
-            : fVTable(GetInlineVTable<Fn>()) {
-        // We've got a functor that fits in a pointer.  We copy it right inline.
-        fFunction = NULL;  // Quiets a (spurious) warning that fFunction might be uninitialized.
-        SkNEW_PLACEMENT_ARGS(&fFunction, Fn, (Forward(fn)));
-    }
+    SkFunction(R (*fn)(Args...)) : fFunction(SkNEW_ARGS(FnPtrImpl, (fn))) {}
 
-    ~SkFunction() { fVTable.fCleanUp(fFunction); }
+    SkFunction(const SkFunction& other) { *this = other; }
+    SkFunction& operator=(const SkFunction& other) {

[bungeman-skia, 2015/05/06 14:16:31]

Check for self assignment?

[mtklein, 2015/05/06 14:22:10]

Done.

+        if (other.fFunction) {

[bungeman-skia, 2015/05/06 14:18:26]

If the other guy doesn't have a function, shouldn't this not have a function as
well?

[mtklein, 2015/05/06 14:22:10]

Ooh, yes, done.  As you might imagine, that's a tricky situation to unit test.

+            fFunction.reset(other.fFunction->clone());
+        }
+        return *this;
+    }
 
-    R operator()(Args... args) { return fVTable.fCall(fFunction, Forward(args)...); }
+    R operator()(Args... args) const {
+        SkASSERT(fFunction.get());
+        return fFunction->call(Forward(args)...);
+    }
 
 private:
     // ~= std::forward.  This moves its argument if possible, falling back to a copy if not.
     template <typename T> static T&& Forward(T& v) { return (T&&)v; }
 
-    struct VTable {
-        R (*fCall)(void*, Args...);
-        void (*fCleanUp)(void*);
+    struct Interface {
+        virtual ~Interface() {}
+        virtual R call(Args...) const = 0;
+        virtual Interface* clone() const = 0;
     };
 
-    // Used when fFunction is a function pointer of type R(*)(Args...).
-    static const VTable& GetFunctionPointerVTable() {
-        static const VTable vtable = {
-            [](void* fn, Args... args) {
-                return reinterpret_cast<R(*)(Args...)>(fn)(Forward(args)...);
-            },
-            [](void*) { /* Nothing to clean up for function pointers. */ }
-        };
-        return vtable;
-    }
-
-    // Used when fFunction is a pointer to a functor of type Fn on the heap (we own it).
     template <typename Fn>
-    static const VTable& GetOutlineVTable() {
-        static const VTable vtable = {
-            [](void* fn, Args... args) { return (*static_cast<Fn*>(fn))(Forward(args)...); },
-            [](void* fn) { SkDELETE(static_cast<Fn*>(fn)); },
-        };
-        return vtable;
-    }
+    class LambdaImpl final : public Interface {
+    public:
+        LambdaImpl(const Fn& fn) : fFn(fn) {}
+
+        R call(Args... args) const override { return fFn(Forward(args)...); }
+        Interface* clone() const { return SkNEW_ARGS(LambdaImpl<Fn>, (fFn)); }
+    private:
+        Fn fFn;
+    };
 
-    // Used when fFunction _is_ a functor of type Fn, not a pointer to the functor.
-    template <typename Fn>
-    static const VTable& GetInlineVTable() {
-        static const VTable vtable = {
-            [](void* fn, Args... args) {
-                union { void** p; Fn* f; } pun = { &fn };
-                return (*pun.f)(Forward(args)...);
-            },
-            [](void* fn) {
-                union { void** p; Fn* f; } pun = { &fn };
-                (*pun.f).~Fn();
-                (void)(pun.f);   // Otherwise, when ~Fn() is trivial, MSVC complains pun is unused.
-            }
-        };
-        return vtable;
-    }
+    class FnPtrImpl final : public Interface {
+    public:
+        FnPtrImpl(R (*fn)(Args...)) : fFn(fn) {}
 
+        R call(Args... args) const override { return fFn(Forward(args)...); }
+        Interface* clone() const { return SkNEW_ARGS(FnPtrImpl, (fFn)); }
+    private:
+        R (*fFn)(Args...);
+    };
 
-    void* fFunction;        // A function pointer, a pointer to a functor, or an inlined functor.
-    const VTable& fVTable;  // How to call, delete (and one day copy, move) fFunction.
+    SkAutoTDelete<Interface> fFunction;
 };
 
-// TODO:
-//   - is it worth moving fCall out of the VTable into SkFunction itself to avoid the indirection?
-//   - make SkFunction copyable
-
 #endif//SkFunction_DEFINED