1162029 - enable returning a result from nsRunnableMethodArguments::apply

Status: NEW

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

I want to use nsRunnableMethodArguments in media/mtransport/'s WrapRunnable* helper classes.  But WrapRunnable*Ret can return values to the dispatching thread, so nsRunnableMethodArguments::apply needs to be generalized to return values, not just void.

I'm not entirely sure of the best way to do this, given that we don't have a lot of fancy STL features at our disposal.  Patch coming up, which I'm sure can be improved.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - refactor common parts of nsRunnableMethodArguments::apply into RunnableMethodCall

This is a reasonable cleanup on its own, I think.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - enable nsRunnableMethodArguments::apply to return a value

This is the part that I think there ought to be an easier way to do.  But it
was the best way I could think of to handle the situation the comment above
RunnableMethodCall describes.  Curious if you have any template feedback on
this one.

I'm not even sure this will be needed to handle the WrapRunnable stuff in
media/mtransport/, but this might be a good opportunity to learn something
about template writing!

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8602113 [details] [diff] [review]
part 1 - refactor common parts of nsRunnableMethodArguments::apply into RunnableMethodCall

Review of attachment 8602113 [details] [diff] [review]:
-----------------------------------------------------------------

I think now that we have variadic templates, we can just give the primary nsRunnableMethodArguments template a single implementation, rather than specializing it up to a set number of arguments. We can leave that for a follow-up, though.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8602114 [details] [diff] [review]
part 2 - enable nsRunnableMethodArguments::apply to return a value

Review of attachment 8602114 [details] [diff] [review]:
-----------------------------------------------------------------

Would something like this work?

struct RunnableMethodCall
{
  template<typename Class, typename Method, typename... Args>
  static auto apply(Class* aObj, Method aMethod, Args&... args)	
      -> decltype(((*aObj).*aMethod)(args.PassAsParameter()...))
  {
    return ((*aObj).*aMethod)(args.PassAsParameter()...);
  }	
};

(and then no changes to nsRunnableMethodImpl::Run() should be necessary).

::: xpcom/glue/nsThreadUtils.h
@@ +659,2 @@
>    {
> +    ::detail::RunnableMethodCall<R>::apply(o, m);

You'll need to add 'return' to these sites.

Comment 5

[Botond Ballo [:botond]]

Attachment: Part 2 - suggested approach

Here's what I had in mind in comment 4. I tried it out and it seems to be working (though I haven't tested it with the uses you had in mind for WrapRunnable).

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] [at c++ standards meeting May 4-9] from comment #3)
> I think now that we have variadic templates, we can just give the primary
> nsRunnableMethodArguments template a single implementation, rather than
> specializing it up to a set number of arguments. We can leave that for a
> follow-up, though.

I filed bug 1163320 for this.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8603738 [details] [diff] [review]
Part 2 - suggested approach

Review of attachment 8603738 [details] [diff] [review]:
-----------------------------------------------------------------

It's kinda bleh that we have to repeat the calling expression twice (once for the return type, once for the actual return expression).  I like my way a little more, since--well, at least for the usages I have--you always have a return type known.  Maybe it matters less if we had proper tuples, and then we'd only be repeating ourselves once or twice?

But none of this matters terribly unless the blocked bug is actually willing to move forward with the proposed approach.  Let me see where we stand with that.

Comment 8

[Botond Ballo [:botond]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #7)
> It's kinda bleh that we have to repeat the calling expression twice (once
> for the return type, once for the actual return expression).  

Yeah, this is one of C++11's warts. C++14 fixes this by allowing you to simply write:

auto function(arguments)
{
    return expression;  // return type is deduced from type of 'expression'
}

> Maybe it matters less if we had proper tuples, and
> then we'd only be repeating ourselves once or twice?

That's exactly what I'm trying to do in bug 1163320.

Comment 9

[Botond Ballo [:botond]]

What's the status of this bug? Do we still want to unify the helpers in runnable_utils.h (RunnableMethodCallHelper) and the ones in nsThreadUtils.h (RunnableMethodArguments)?

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Botond Ballo [:botond] from comment #9)
> What's the status of this bug? Do we still want to unify the helpers in
> runnable_utils.h (RunnableMethodCallHelper) and the ones in nsThreadUtils.h
> (RunnableMethodArguments)?

Yes.