rtc::FunctionView improvements: accept function pointers and nullptr

webrtc/base/function_view.h

Index: webrtc/base/function_view.h
diff --git a/webrtc/base/function_view.h b/webrtc/base/function_view.h
index 6a7c9109ca6d906ff53601799c65b208917200e4..861bccff3e09166020c65298b7caa74354f4a37f 100644
--- a/webrtc/base/function_view.h
+++ b/webrtc/base/function_view.h
@@ -14,16 +14,27 @@
 #include <type_traits>
 #include <utility>
 
+#include "webrtc/base/checks.h"
+
 // Just like std::function, FunctionView will wrap any callable and hide its
 // actual type, exposing only its signature. But unlike std::function,
 // FunctionView doesn't own its callable---it just points to it. Thus, it's a
 // good choice mainly as a function argument when the callable argument will
 // not be called again once the function has returned.
 //
-// TODO(kwiberg): FunctionView doesn't work with function pointers, just with
-// lambdas. It's trivial to work around this by wrapping the function pointer
-// in a stateless lambda, but it's tedious so it'd be nice to not have to do
-// it.
+// Its constructors are implicit, so that callers won't have to convert lambdas
+// and other callables to FunctionView<Blah(Blah, Blah)> explicitly. This is
+// safe because FunctionView is only a reference to the real callable.
+//
+// Example use:
+//
+//   void SomeFunction(rtc::FunctionView<int(int)> index_transform);
+//   ...
+//   SomeFunction([](int i) { return 2 * i + 1; });
+//
+// Note: FunctionView is tiny (essentially just two pointers) and trivially
+// copyable, so it's probably cheaper to pass it by value than by const
+// reference.
 
 namespace rtc {
 
@@ -33,36 +44,85 @@ class FunctionView;  // Undefined.
 template <typename RetT, typename... ArgT>
 class FunctionView<RetT(ArgT...)> final {
  public:
-  // This constructor is implicit, so that callers won't have to convert
-  // lambdas and other callables to FunctionView<Blah(Blah, Blah)> explicitly.
-  // This is safe because FunctionView is only a reference to the real
-  // callable.
-  //
-  // We jump through some template metaprogramming hoops to ensure that this
-  // constructor does *not* accept FunctionView arguments. That way, copy
-  // construction, assignment, swap etc. will all do the obvious thing (because
-  // they use the implicitly-declared copy constructor and copy assignment),
-  // and we will never get a FunctionView object that points to another
-  // FunctionView.
-  template <typename F,
-            typename std::enable_if<!std::is_same<
-                FunctionView,
-                typename std::remove_cv<typename std::remove_reference<
-                    F>::type>::type>::value>::type* = nullptr>
+  // Constructor for lambdas and other callables; it accepts every type of
+  // argument except those noted in its enable_if call.
+  template <
+      typename F,
+      typename std::enable_if<
+          // Not for function pointers; we have another constructor for that
+          // below.
+          !std::is_function<typename std::remove_pointer<
+              typename std::remove_reference<F>::type>::type>::value &&
+
+          // Not for nullptr; we have another constructor for that below.
+          !std::is_same<std::nullptr_t,
+                        typename std::remove_cv<F>::type>::value &&
+
+          // Not for FunctionView objects; we have another constructor for that
+          // (the implicitly declared copy constructor).
+          !std::is_same<FunctionView,
+                        typename std::remove_cv<typename std::remove_reference<
+                            F>::type>::type>::value>::type* = nullptr>
+  FunctionView(F&& f)
+      : call_(CallVoidPtr<typename std::remove_reference<F>::type>) {
+    f_.void_ptr = &f;
+  }
+
+  // Constructor that accepts function pointers. If the argument is null, the
+  // result is an empty FunctionView.
+  template <
+      typename F,
+      typename std::enable_if<std::is_function<typename std::remove_pointer<
+          typename std::remove_reference<F>::type>::type>::value>::type* =
+          nullptr>
   FunctionView(F&& f)
-      : f_(&f), call_(Call<typename std::remove_reference<F>::type>) {}
+      : call_(f ? CallFunPtr<typename std::remove_pointer<F>::type> : nullptr) {

[kwiberg-webrtc, 2016/09/29 07:52:40]

We incur the overhead of testing for null both if the caller gave us a function
pointer and if she gave us a function that decayed to a function pointer. I
suspect it may be possible to catch those two cases with two separate
constructors, so that we could skip the check in the second one, but I haven't
been able to make it work.

It shouldn't be too bad, though. I haven't checked, but I suspect the compiler
can do this with a conditional move or something, instead of a full-blown
conditional jump.

[ossu, 2016/10/03 11:28:12]

Won't this all be inlined and then the compiler can just remove the check in any
cases where the address is a known constant? Regardless, a check for null is
fine. I bet there are bigger fish to fry in terms of performance.

[kwiberg-webrtc, 2016/10/03 11:46:31]

That's... probably right. Why didn't I think of that? Thanks!
Indeed.

+    f_.fun_ptr = reinterpret_cast<void (*)()>(f);
+  }
+
+  // Constructor that accepts nullptr. It creates an empty FunctionView.
+  template <typename F,
+            typename std::enable_if<std::is_same<
+                std::nullptr_t,
+                typename std::remove_cv<F>::type>::value>::type* = nullptr>
+  FunctionView(F&& f) : call_(nullptr) {}
+
+  // Default constructor. Creates an empty FunctionView.
+  FunctionView() : call_(nullptr) {}
 
   RetT operator()(ArgT... args) const {
+    RTC_DCHECK(call_);
     return call_(f_, std::forward<ArgT>(args)...);
   }
 
+  // Returns true if we have a function, false if we don't (i.e., we're null).
+  explicit operator bool() const { return !!call_; }
+
  private:
+  union VoidUnion {
+    void* void_ptr;
+    void (*fun_ptr)();
+  };
+
+  template <typename F>
+  static RetT CallVoidPtr(VoidUnion vu, ArgT... args) {
+    return (*static_cast<F*>(vu.void_ptr))(std::forward<ArgT>(args)...);
+  }
   template <typename F>
-  static RetT Call(void* f, ArgT... args) {
-    return (*static_cast<F*>(f))(std::forward<ArgT>(args)...);
+  static RetT CallFunPtr(VoidUnion vu, ArgT... args) {
+    return (reinterpret_cast<typename std::add_pointer<F>::type>(vu.fun_ptr))(
+        std::forward<ArgT>(args)...);
   }
-  void* f_;
-  RetT (*call_)(void* f, ArgT... args);
+
+  // A pointer to the callable thing, with type information erased. It's a
+  // union because we have to use separate types depending on if the callable
+  // thing is a function pointer or something else.
+  VoidUnion f_;
+
+  // Pointer to a dispatch function that knows the type of the callable thing
+  // that's stored in f_, and how to call it. A FunctionView object is empty
+  // (null) iff call_ is null.
+  RetT (*call_)(VoidUnion, ArgT...);
 };
 
 }  // namespace rtc