Closed Bug 622728 Opened 10 years ago Closed 7 years ago

Want NS_NewRunnableMethodWithArg

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: khuey, Assigned: valentin)

References

Details

Attachments

(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 {
  NS_DECL_ISUPPORTS
  NS_DECL_NSIFOO
  nsresult DoFoo(nsIBar* aBar);
};

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

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]
Patch

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.

NS_NewRunnableMethodWithArg<nsCOMPtr<nsIFoo>>(...)

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]
runnable1.patch

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.
Thanks!
Attachment #500965 - Attachment is obsolete: true
Attachment #793134 - Attachment is obsolete: true
Attachment #794916 - Flags: review?(benjamin)
Ping
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.

@Benjamin
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.
r=bsmedberg
https://tbpl.mozilla.org/?tree=Try&rev=b41a26fc15c4
Attachment #515711 - Attachment is obsolete: true
Attachment #800251 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8c564bfcd0ec
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment on attachment 800960 [details] [diff] [review]
runnableWithArg.patch final

>diff --git a/xpcom/tests/moz.build b/xpcom/tests/moz.build
>--- a/xpcom/tests/moz.build
>+++ b/xpcom/tests/moz.build
>@@ -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.