Closed Bug 622728 Opened 10 years ago Closed 7 years ago

Want NS_NewRunnableMethodWithArg


(Core :: XPCOM, enhancement)

Not set





(Reporter: khuey, Assigned: valentin)




(1 file, 6 obsolete files)

Would be nice to able to pass arguments to a runnable created through NS_NewRunnableMethod.
Thanks for the link; it was good reading.

What I had in mind was something like:

class nsFoo : public nsIFoo {
  nsresult DoFoo(nsIBar* aBar);

void Method()
  nsCOMPtr<nsIFoo> foo = GetFoo();
  nsCOMPtr<nsIBar> bar = GetBar();
  nsCOMPtr<nsIRunnable> runnable =
    NS_NewRunnableMethodWithArg(foo, &nsFoo::DoFoo, bar);

The main issue I see with a naive implementation (e.g. mem_fun1 from the STL) is that if the argument is deduced from the function signature it would be an nsIBar*, and we'd almost certainly want the runnable to maintain an nsCOMPtr<nsIBar> instead.
Attached patch Patch (obsolete) — Splinter Review
This patch contains a test and an example converted usage.

I'm not sure that all of the template changes are necessary; I just fiddled with things until they compiled :-/
Attachment #500965 - Flags: review?(benjamin)
Also, the stdcall stuff here is a NOOP because the configure check isn't run on windows, but that's neither here nor there at the moment.
Comment on attachment 500965 [details] [diff] [review]

I've rejected a more comprehensive form of this before because the implicit instantiation is really dangerous. For example, if you're passing around interface pointers, you usually want to instantiate this with nsCOMPtr<nsIFoo> and not store a raw nsIFoo*. I would mind it if we could find a way to require explicit instantiation, e.g.


If we can't find a way to do that, I really would prefer the longhand version, or a more explicit class instantiation.
Attachment #500965 - Flags: review?(benjamin) → review-
Attached file Example of catching pointers (obsolete) —
I should've thought of this sooner. Basically you add a T* overload for your template function, which catches calls with raw pointers (e.g. nsIFoo* -> T=nsIFoo) and make that fail to compile somehow (see example). The base version catches everything that's not a pointer, unless you explicitly make T be a pointer type, e.g. nsIFoo*. The overloaded version doesn't come into play 'coz it's looking for (in this case) nsIFoo**.
Attachment #515711 - Attachment mime type: application/octet-stream → text/plain
Attached patch runnable1.patch (obsolete) — Splinter Review
Hi Benjamin,
I tweaked Kyle's patch a bit. A gdb breakpoint confirms that nsRunnableMethodReceiver(nsCOMPtr<ClassType> obj, Arg arg) is being called.
Could you confirm this is correct before I clean up the patch and add some tests? Thanks!
Attachment #793134 - Flags: feedback?(benjamin)
Comment on attachment 793134 [details] [diff] [review]

I don't understand how this is substantially different than the first time around; what prevents you from instantiating this with a raw pointer type?
Attachment #793134 - Flags: feedback?(benjamin)
From #c5 I understand the problem is that it takes the raw pointer if you give it an nsCOMPtr, so I made it save the nsCOMPtr - The main difference is the explicit use of constructors that take the nsCOMPtr and save it.
But maybe I don't understand the requirements correctly.
My comment was not specifically about interfaces/nsCOMPtr; it is about being explicit because ownership from implicit templates is going to be tricky.

So I'll take a patch if we can require the template to be explicit (NS_NewRunnableMethodWithArg<nsCOMPtr<nsIFoo> >(...) but not otherwise.
Thanks, I think I understand now. I'll try to fix the patch tonight.
Also, do you also want NS_NewRunnableMethod() to be explicit?
I don't understand the question.
Do we want the existing NS_NewRunnableMetod to require an explicit template type? Or can it continue to be used as it is right now?
If there is no extra argument, then no it doesn't need a template type (nor does it need to be a template). Only the extra-argument form needs to be explicit.
Attached patch runnableWithArg.patch (obsolete) — Splinter Review
Turns out that Peter's example illustrates a great way to deal with the problems.
Attachment #500965 - Attachment is obsolete: true
Attachment #793134 - Attachment is obsolete: true
Attachment #794916 - Flags: review?(benjamin)
Attached patch runnableWithArg.patch v3 (obsolete) — Splinter Review
Cleaned up patch and added tests.
Assignee: khuey → valentin.gosu
Attachment #794916 - Attachment is obsolete: true
Attachment #794916 - Flags: review?(benjamin)
Attachment #797521 - Flags: review?(benjamin)
Blocks: 904594
Comment on attachment 797521 [details] [diff] [review]
runnableWithArg.patch v3

This patch appears to still implement the "owning" boolean which is not the desired form. And this code should not compile:

+        nsCOMPtr<nsIRunnable> event =
+            NS_NewRunnableMethodWithArg(this, &Dashboard::GetConnectionStatus, status);

It should only compile with an explicit template:

nsCOMPtr<nsIRunnable> event =
  NS_NewRunnableMethodWithArg<ConnStatus>(this, &Dashboard::GetConnectionStatus, stats);

Then you can get rid of all the owning/nonowning madness and if people want an owning runnable it can just be:

NS_NewRunnableMethodWithArg< nsCOMPtr<nsIFoo> >(this, &Method, obj);
Attachment #797521 - Flags: review?(benjamin) → review-
This patch makes it require an explicit template only for raw pointer types. Any idea how I make it _always_ require a template?
<Waldo> bsmedberg: |template<typename T, typename U = T> void SomeFunction(U u) { static_assert(mozilla::IsSame<T, U>::value, "must provide T as same type as U"); ... }| maybe?
<Waldo> a bit hackish if it works, but
<bsmedberg> that's sneaky...
That happens to require C++11, which we compile with, but I don't know if all our compilers (well, more or less MSVC, I bet) support that.  Making it a templated method of a templated class is a fully-portable workaround:

template<typename T>
struct NewRunnableMethodWithArg
  template<typename U>
  static void call(U u)
    static_assert(mozilla::IsSame<T, U>::value, "argument type must match specified type");

or somesuch like that, so that NewRunnableMethodWithArg<T>::call(not a T) will fail the static_assert.
It might be possible to do something with dependent types, e.g.

template<typename T>
struct dependent_type
  typedef T type;

template<typename T>
void SomeFunction(dependent_type<T>::type t)

Since dependent_type<T>::type is dependent on T, the compiler can't infer it, so you have to explicitly provide it, even though it's the same type as T.
Attached patch runnableWithArg.patch v4 (obsolete) — Splinter Review
Thanks Neil, your suggestion seems to work and it's pretty minimal.

The patch now requires explicit template type parameter for the argument. The owning part is still there, as it's required for "NonOwning runnables".

Hope this is ok.
Attachment #797521 - Attachment is obsolete: true
Attachment #800251 - Flags: review?(benjamin)
Comment on attachment 800251 [details] [diff] [review]
runnableWithArg.patch v4

Awesome, thank you!
Attachment #800251 - Flags: review?(benjamin) → review+
Rebased against m-c.
Attachment #515711 - Attachment is obsolete: true
Attachment #800251 - Attachment is obsolete: true
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment on attachment 800960 [details] [diff] [review]
runnableWithArg.patch final

>diff --git a/xpcom/tests/ b/xpcom/tests/
>--- a/xpcom/tests/
>+++ b/xpcom/tests/
>@@ -73,16 +73,17 @@ CPP_UNIT_TESTS += [
>     'TestHashtables.cpp',
>     'TestID.cpp',
>     'TestObserverArray.cpp',
>     'TestObserverService.cpp',
>     'TestPipe.cpp',
>     'TestRefPtr.cpp',
>     'TestTArray.cpp',
>     'TestTextFormatter.cpp',
>+    'TestThreadUtils.cpp'
You must put the trailing comma here, otherwise someone will need to edit the line when they add a new test.
You need to log in before you can comment on or make changes to this bug.