From 0d992db1cf5003340409ed02993266004cc0a57c Mon Sep 17 00:00:00 2001 From: mtklein Date: Wed, 6 May 2015 07:40:25 -0700 Subject: [PATCH] Make SkFunction copyable so it can go in containers. This totally overhauls the implementation to use ordinary inheritance-based type erasure. I give up for now getting my manual vtable shenanigans to work with MSVC. Still those same "expected ; before ), also expected ) before ;" errors. I added support for uninitialized SkFunctions and operator=(), because it was fairly straightforward with this implementation. The main downside here is that I've removed the inline implementation. All SkFunctions involve a heap allocation, even when just wrapping function pointers. BUG=skia: Review URL: https://codereview.chromium.org/1056673002 --- src/core/SkFunction.h | 104 ++++++++++++++++------------------------- tests/FunctionTest.cpp | 49 +++++++++++++------ 2 files changed, 73 insertions(+), 80 deletions(-) diff --git a/src/core/SkFunction.h b/src/core/SkFunction.h index 43438dd9e2..429c6f5ade 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 class SkFunction; template -class SkFunction : SkNoncopyable { +class SkFunction { public: - SkFunction(R (*fn)(Args...)) : fVTable(GetFunctionPointerVTable()) { - // We've been passed a function pointer. We'll just store it. - fFunction = reinterpret_cast(fn); - } + SkFunction() {} template - SkFunction(Fn fn, SK_WHEN_C((sizeof(Fn) > sizeof(void*)), void*) = nullptr) - : fVTable(GetOutlineVTable()) { - // 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))) {} + + SkFunction(R (*fn)(Args...)) : fFunction(SkNEW_ARGS(FnPtrImpl, (fn))) {} + + SkFunction(const SkFunction& other) { *this = other; } + SkFunction& operator=(const SkFunction& other) { + if (this != &other) { + fFunction.reset(other.fFunction ? other.fFunction->clone() : nullptr); + } + return *this; } - template - SkFunction(Fn fn, SK_WHEN_C((sizeof(Fn) <= sizeof(void*)), void*) = nullptr) - : fVTable(GetInlineVTable()) { - // 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))); + R operator()(Args... args) const { + SkASSERT(fFunction.get()); + return fFunction->call(Forward(args)...); } - ~SkFunction() { fVTable.fCleanUp(fFunction); } - - R operator()(Args... args) { return fVTable.fCall(fFunction, Forward(args)...); } - private: // ~= std::forward. This moves its argument if possible, falling back to a copy if not. template 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(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 - static const VTable& GetOutlineVTable() { - static const VTable vtable = { - [](void* fn, Args... args) { return (*static_cast(fn))(Forward(args)...); }, - [](void* fn) { SkDELETE(static_cast(fn)); }, - }; - return vtable; - } + class LambdaImpl final : public Interface { + public: + LambdaImpl(const Fn& fn) : fFn(fn) {} - // Used when fFunction _is_ a functor of type Fn, not a pointer to the functor. - template - 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; - } + R call(Args... args) const override { return fFn(Forward(args)...); } + Interface* clone() const { return SkNEW_ARGS(LambdaImpl, (fFn)); } + private: + Fn fFn; + }; + class FnPtrImpl final : public Interface { + public: + FnPtrImpl(R (*fn)(Args...)) : fFn(fn) {} - 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. + R call(Args... args) const override { return fFn(Forward(args)...); } + Interface* clone() const { return SkNEW_ARGS(FnPtrImpl, (fFn)); } + private: + R (*fFn)(Args...); + }; + + SkAutoTDelete fFunction; }; -// TODO: -// - is it worth moving fCall out of the VTable into SkFunction itself to avoid the indirection? -// - make SkFunction copyable - #endif//SkFunction_DEFINED diff --git a/tests/FunctionTest.cpp b/tests/FunctionTest.cpp index 0feef7c04c..cf6b78905f 100644 --- a/tests/FunctionTest.cpp +++ b/tests/FunctionTest.cpp @@ -8,29 +8,31 @@ #include "SkFunction.h" #include "Test.h" -static void test_add_five(skiatest::Reporter* r, SkFunction&& f) { +static void test_add_five(skiatest::Reporter* r, SkFunction& f) { REPORTER_ASSERT(r, f(3) == 8); REPORTER_ASSERT(r, f(4) == 9); } +static void test_add_five(skiatest::Reporter* r, SkFunction&& f) { test_add_five(r, f); } + static int add_five(int x) { return x + 5; } struct AddFive { - int operator()(int x) { return x + 5; }; + int operator()(int x) const { return x + 5; }; }; -class MoveOnlyAdd5 : SkNoncopyable { +class MoveOnlyThree : SkNoncopyable { public: - MoveOnlyAdd5() {} - MoveOnlyAdd5(MoveOnlyAdd5&&) {} - MoveOnlyAdd5& operator=(MoveOnlyAdd5&&) { return *this; } + MoveOnlyThree() {} + MoveOnlyThree(MoveOnlyThree&&) {} + MoveOnlyThree& operator=(MoveOnlyThree&&) { return *this; } - int operator()(int x) { return x + 5; } + int val() { return 3; } }; DEF_TEST(Function, r) { - // We should be able to turn a static function, an explicit functor, or a lambda - // all into an SkFunction equally well. + // We should be able to turn a function pointer, an explicit functor, or a + // lambda into an SkFunction all equally well. test_add_five(r, &add_five); test_add_five(r, AddFive()); test_add_five(r, [](int x) { return x + 5; }); @@ -40,12 +42,29 @@ DEF_TEST(Function, r) { int a = 1, b = 1, c = 1, d = 1, e = 1; test_add_five(r, [&](int x) { return x + a + b + c + d + e; }); - // Makes sure we forward the functor when constructing SkFunction. - test_add_five(r, MoveOnlyAdd5()); - // Makes sure we forward arguments when calling SkFunction. - SkFunction f([](int x, MoveOnlyAdd5&& addFive, int y) { - return x * addFive(y); + SkFunction f([](int x, MoveOnlyThree&& three, int y) { + return x * three.val() + y; }); - REPORTER_ASSERT(r, f(2, MoveOnlyAdd5(), 4) == 18); + REPORTER_ASSERT(r, f(2, MoveOnlyThree(), 4) == 10); + + // SkFunctions can go in containers. + SkTArray> add_fivers; + add_fivers.push_back(&add_five); + add_fivers.push_back(AddFive()); + add_fivers.push_back([](int x) { return x + 5; }); + add_fivers.push_back([&](int x) { return x + a + b + c + d + e; }); + for (auto& f : add_fivers) { + test_add_five(r, f); + } + + // SkFunctions are assignable. + SkFunction empty; + empty = [](int x) { return x + 5; }; + test_add_five(r, empty); + + // This all is silly acrobatics, but it should at least work correctly. + SkFunction emptyA, emptyB(emptyA); + emptyA = emptyB; + emptyA = emptyA; }