rtc::FunctionView improvements: accept function pointers and nullptr

rtc::FunctionView improvements: accept function pointers and nullptr

BUG=webrtc:5801
Committed: https://crrev.com/607d9d7f604f7d8cc4a427de38db06eb674482b9
Cr-Commit-Position: refs/heads/master@{#14483}

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

kwiberg@webrtc.org changed reviewers:
+ ossu@webrtc.org

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

Template Thursday!

https://codereview.webrtc.org/2375023004/diff/1/webrtc/base/function_view.h
File webrtc/base/function_view.h (right):

https://codereview.webrtc.org/2375023004/diff/1/webrtc/base/function_view.h#n...
webrtc/base/function_view.h:79: : call_(f ? CallFunPtr<typename
std::remove_pointer<F>::type> : nullptr) {
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]

lgtm

https://codereview.webrtc.org/2375023004/diff/1/webrtc/base/function_view.h
File webrtc/base/function_view.h (right):

https://codereview.webrtc.org/2375023004/diff/1/webrtc/base/function_view.h#n...
webrtc/base/function_view.h:79: : call_(f ? CallFunPtr<typename
std::remove_pointer<F>::type> : nullptr) {
On 2016/09/29 07:52:40, kwiberg-webrtc wrote:
> 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.

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]

https://codereview.webrtc.org/2375023004/diff/1/webrtc/base/function_view.h
File webrtc/base/function_view.h (right):

https://codereview.webrtc.org/2375023004/diff/1/webrtc/base/function_view.h#n...
webrtc/base/function_view.h:79: : call_(f ? CallFunPtr<typename
std::remove_pointer<F>::type> : nullptr) {
On 2016/10/03 11:28:12, ossu wrote:
> On 2016/09/29 07:52:40, kwiberg-webrtc wrote:
> > 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.
> 
> 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?

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

> Regardless, a check for null is
> fine. I bet there are bigger fish to fry in terms of performance.

Indeed.

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

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-10-03 11:46:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-03 12:38:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 12:38:30]

Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/13636)

[kwiberg-webrtc, 2016-10-03 17:31:53]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-10-03 17:32:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-03 19:40:40]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-03 19:40:47]

Description was changed from

==========
rtc::FunctionView improvements: accept function pointers and nullptr

BUG=webrtc:5801
==========

to

==========
rtc::FunctionView improvements: accept function pointers and nullptr

BUG=webrtc:5801

Committed: https://crrev.com/607d9d7f604f7d8cc4a427de38db06eb674482b9
Cr-Commit-Position: refs/heads/master@{#14483}
==========

[commit-bot: I haz the power, 2016-10-03 19:40:48]

Patchset 1 (id:??) landed as
https://crrev.com/607d9d7f604f7d8cc4a427de38db06eb674482b9
Cr-Commit-Position: refs/heads/master@{#14483}