Kris Maglione [:kmag] Assignee
	Description • 6 years ago

The IterableIterator helpers currently assume that key/value getters always return non-JSAPI values that can be coerced using ToJSValue. In bug 1463587, though, I need to return a JS::Value directly, and I need a JSContext to create it (which requires some ugly hacks).

This should be easy enough to do with some template specializations.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 6 years ago
mozreview-review

Comment on attachment 8991496 [details]
Bug 1472342: Part 1 - Properly support iterator getters which need JSContexts.

https://reviewboard.mozilla.org/r/256386/#review263512

::: dom/bindings/IterableIterator.h:63
(Diff revision 1)
>  };
>  
> +// Helpers to call iterator getter methods with the correct arguments, depending
> +// on the types they return, and convert the result to JS::Values.
> +
> +// Helper for Get[Key,Value]At(uint32_t) methods, which accept an index and

AtIndex, right?

::: dom/bindings/IterableIterator.h:66
(Diff revision 1)
> +// on the types they return, and convert the result to JS::Values.
> +
> +// Helper for Get[Key,Value]At(uint32_t) methods, which accept an index and
> +// return a type supported by ToJSValue.
> +template <typename T, typename F,
> +         typename = decltype((DeclVal<T>().*(F T::*)0)(0))>

So I was trying to figure out why we need all the complication of the decltype/DeclVal business.  It looks like we're trying to enforce the "takes one arg or takes 3 args" thing in the template syntax itself, but it might be simpler, or at least more readable, to just not worry about that there and let SFINAE and the fact that more-specialized templates take precedence over less-specialized ones.

So for example, I think we can do this:

    template <typename T, typename F>
    bool
    CallIterableGetter(JSContext* aCx, F T::* aMethod, T* aInst,
                       uint32_t aIndex,
                       JS::MutableHandle<JS::Value> aResult)
    {
      return ToJSValue(aCx, ((*aInst).*aMethod)(aIndex), aResult);
    }

    template <typename T>
    bool
    CallIterableGetter(JSContext* aCx,
                       bool (T::* aMethod)(JSContext*, uint32_t, JS::MutableHandle<JS::Value>),
                       T* aInst,
                       uint32_t aIndex,
                       JS::MutableHandle<JS::Value> aResult)
    {
      return ((*aInst).*aMethod)(aCx, aIndex, aResult);
    }

and it will almost do the right thing.  The only remaining issue is that we want a second copy of that second method with the second arg a const pointer-to-method:

    template <typename T>
    bool
    CallIterableGetter(JSContext* aCx,
                       bool (T::* aMethod)(JSContext*, uint32_t, JS::MutableHandle<JS::Value>) const,
                       T* aInst,
                       uint32_t aIndex,
                       JS::MutableHandle<JS::Value> aResult)
    {
      return ((*aInst).*aMethod)(aCx, aIndex, aResult);
    }

and I don't see an obvious way to avoid that duplication without some of the decltype bits.  We could leave it out and make the relevant methods non-const, of course...

Anyway, thoughts on the tradeoff there?

If we do keep the decltype bits, I think they're worth a comment explaining what they're trying to do, because they're not exactly easy to read.

::: dom/bindings/IterableIterator.h:66
(Diff revision 1)
> +// on the types they return, and convert the result to JS::Values.
> +
> +// Helper for Get[Key,Value]At(uint32_t) methods, which accept an index and
> +// return a type supported by ToJSValue.
> +template <typename T, typename F,
> +         typename = decltype((DeclVal<T>().*(F T::*)0)(0))>

Indent is off by 1.

::: dom/bindings/IterableIterator.h:75
(Diff revision 1)
> +                   JS::MutableHandle<JS::Value> aResult)
> +{
> +  return ToJSValue(aCx, ((*aInst).*aMethod)(aIndex), aResult);
> +}
> +
> +// Helper for Get[Key,Value]At(JSContext*, uint32_t, MutableHandleValue) methods, which accept

AtIndex

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 6 years ago
mozreview-review

Comment on attachment 8991497 [details]
Bug 1472342: Part 2 - Update SharedMap iterator methods to use provided JSContext.

https://reviewboard.mozilla.org/r/256388/#review263554

::: dom/ipc/SharedMap.cpp:187
(Diff revision 1)
> -    return JS::NullValue();
> -  }
> -
> -  AutoJSContext cx;
>  
> -  JSAutoRealm ar(cx, wrapper);
> +  EntryArray()[aIndex]->Read(aCx, aResult, IgnoreErrors());

I think it would be clearer to explicitly return null (or undefined?) if there is an error.  Or even to throw that error as an exception?

	Kris Maglione [:kmag] Assignee
	Comment 5 • 6 years ago
mozreview-review-reply

Comment on attachment 8991496 [details]
Bug 1472342: Part 1 - Properly support iterator getters which need JSContexts.

https://reviewboard.mozilla.org/r/256386/#review263512

> So I was trying to figure out why we need all the complication of the decltype/DeclVal business.  It looks like we're trying to enforce the "takes one arg or takes 3 args" thing in the template syntax itself, but it might be simpler, or at least more readable, to just not worry about that there and let SFINAE and the fact that more-specialized templates take precedence over less-specialized ones.
> 
> So for example, I think we can do this:
> 
>     template <typename T, typename F>
>     bool
>     CallIterableGetter(JSContext* aCx, F T::* aMethod, T* aInst,
>                        uint32_t aIndex,
>                        JS::MutableHandle<JS::Value> aResult)
>     {
>       return ToJSValue(aCx, ((*aInst).*aMethod)(aIndex), aResult);
>     }
> 
>     template <typename T>
>     bool
>     CallIterableGetter(JSContext* aCx,
>                        bool (T::* aMethod)(JSContext*, uint32_t, JS::MutableHandle<JS::Value>),
>                        T* aInst,
>                        uint32_t aIndex,
>                        JS::MutableHandle<JS::Value> aResult)
>     {
>       return ((*aInst).*aMethod)(aCx, aIndex, aResult);
>     }
> 
> and it will almost do the right thing.  The only remaining issue is that we want a second copy of that second method with the second arg a const pointer-to-method:
> 
>     template <typename T>
>     bool
>     CallIterableGetter(JSContext* aCx,
>                        bool (T::* aMethod)(JSContext*, uint32_t, JS::MutableHandle<JS::Value>) const,
>                        T* aInst,
>                        uint32_t aIndex,
>                        JS::MutableHandle<JS::Value> aResult)
>     {
>       return ((*aInst).*aMethod)(aCx, aIndex, aResult);
>     }
> 
> and I don't see an obvious way to avoid that duplication without some of the decltype bits.  We could leave it out and make the relevant methods non-const, of course...
> 
> Anyway, thoughts on the tradeoff there?
> 
> If we do keep the decltype bits, I think they're worth a comment explaining what they're trying to do, because they're not exactly easy to read.

I tried to think of some ways to simplify this with specialization, but it wasn't as easy as I'd hoped.

Part of the problem is these aren't two specializations of the same templates. They're two separate template functions, and in your first example, they'd wind up with the same signature, and therefore be ambiguous.

That said, the `typename = decltype((DeclVal<T>().*(F T::*)0)(0))` filter I added to the first method would probably make your version work.

We could probably also make it work with explicit template specialization, but unfortunately, that requires writing out the method type twice, which gets verbose...

Maybe something like this, though?

    template <typename T>
    using IterableGetter = bool (T::* aMethod)(JSContext*, uint32_t, JS::MutableHandle<JS::Value>);

    template <typename T>
    using IterableConstGetter = bool (T::* aMethod)(JSContext*, uint32_t, JS::MutableHandle<JS::Value>) const;

    template <typename T>
    bool
    CallIterableGetter<T, IterableGetter<T>>(
        JSContext* aCx, IterableGetter<T> aMethod, T* aInst,
        uint32_t aIndex, JS::MutableHandle<JS::Value> aResult)
    {
      return ((*aInst).*aMethod)(aCx, aIndex, aResult);
    }

    template <typename T>
    bool
    CallIterableGetter<T, IterableConstGetter<T>>(
        JSContext* aCx, IterableConstGetter<T> aMethod, const T* aInst,
        uint32_t aIndex, JS::MutableHandle<JS::Value> aResult)
    {
      return ((*aInst).*aMethod)(aCx, aIndex, aResult);
    }

It is more verbose than my original attempt, but perhaps easier to follow.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 6 years ago

> They're two separate template functions, and in your first example, they'd wind up with the same signature, and therefore be ambiguous.

I agree that they're not specializations, just two separate base templates, but the base template selection stuff will pick the "most specialized" base template it can, as I understand, which means it will pick the one with only one template arg, if possible, over the one with two template args.  For what it's worth, I tried the code I pasted and it does compile, with both a const and a non-const 3-arg method.  

Your "something like this" won't work because you can't partially specialize a template function (unlike a template class) in C++.

	Kris Maglione [:kmag] Assignee
	Comment 7 • 6 years ago

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> > They're two separate template functions, and in your first example, they'd wind up with the same signature, and therefore be ambiguous.
> 
> I agree that they're not specializations, just two separate base templates,
> but the base template selection stuff will pick the "most specialized" base
> template it can, as I understand, which means it will pick the one with only
> one template arg, if possible, over the one with two template args.  For
> what it's worth, I tried the code I pasted and it does compile, with both a
> const and a non-const 3-arg method.  

Interesting. When I tried something similar, it didn't compile. But perhaps I had two template args. I'll try yours and see how it goes.

	Kris Maglione [:kmag] Assignee
	Comment 8 • 6 years ago
mozreview-review-reply

Comment on attachment 8991496 [details]
Bug 1472342: Part 1 - Properly support iterator getters which need JSContexts.

https://reviewboard.mozilla.org/r/256386/#review263512

> I tried to think of some ways to simplify this with specialization, but it wasn't as easy as I'd hoped.
> 
> Part of the problem is these aren't two specializations of the same templates. They're two separate template functions, and in your first example, they'd wind up with the same signature, and therefore be ambiguous.
> 
> That said, the `typename = decltype((DeclVal<T>().*(F T::*)0)(0))` filter I added to the first method would probably make your version work.
> 
> We could probably also make it work with explicit template specialization, but unfortunately, that requires writing out the method type twice, which gets verbose...
> 
> Maybe something like this, though?
> 
>     template <typename T>
>     using IterableGetter = bool (T::* aMethod)(JSContext*, uint32_t, JS::MutableHandle<JS::Value>);
> 
>     template <typename T>
>     using IterableConstGetter = bool (T::* aMethod)(JSContext*, uint32_t, JS::MutableHandle<JS::Value>) const;
> 
>     template <typename T>
>     bool
>     CallIterableGetter<T, IterableGetter<T>>(
>         JSContext* aCx, IterableGetter<T> aMethod, T* aInst,
>         uint32_t aIndex, JS::MutableHandle<JS::Value> aResult)
>     {
>       return ((*aInst).*aMethod)(aCx, aIndex, aResult);
>     }
> 
>     template <typename T>
>     bool
>     CallIterableGetter<T, IterableConstGetter<T>>(
>         JSContext* aCx, IterableConstGetter<T> aMethod, const T* aInst,
>         uint32_t aIndex, JS::MutableHandle<JS::Value> aResult)
>     {
>       return ((*aInst).*aMethod)(aCx, aIndex, aResult);
>     }
> 
> It is more verbose than my original attempt, but perhaps easier to follow.

I wound up still needing the SFINAE hack for the 1-argument form. Without it, compilation failed with:

 4:13.31 /home/kris/code/mozilla-central/obj-package/dist/include/mozilla/dom/IterableIterator.h:71:51: error: too few arguments to function call, expected 3, have 1
 4:13.31   return ToJSValue(aCx, ((*aInst).*aMethod)(aIndex), aResult);
 4:13.31                          ~~~~~~~~~~~~~~~~~        ^
 4:13.31 /home/kris/code/mozilla-central/obj-package/dom/bindings/MozSharedMapBinding.cpp:475:10: note: in instantiation of function template specialization 'mozilla::dom::CallIterableGetter<mozilla::dom::ipc::SharedMap, bool (JSContext *, unsigned int, JS::MutableHandle<JS::Value>) const>' requested here
 4:13.31     if (!CallIterableGetter(cx, GetValueAtIndex, self, i,
 4:13.31          ^

Which seems to be because I used `const T* aInst` for the const method version, and we pass a non-const pointer. I guess I could stick with a plain `T*`, but I generally prefer maintaining const-correctness to avoiding SFINAE. Or I could just add explicit const and non-const versions of the first method, too...

I changed the 3-argument forms to use what you suggested, though.

	Kris Maglione [:kmag] Assignee
	Comment 9 • 6 years ago
mozreview-review-reply

Comment on attachment 8991496 [details]
Bug 1472342: Part 1 - Properly support iterator getters which need JSContexts.

https://reviewboard.mozilla.org/r/256386/#review263512

> I wound up still needing the SFINAE hack for the 1-argument form. Without it, compilation failed with:
> 
>  4:13.31 /home/kris/code/mozilla-central/obj-package/dist/include/mozilla/dom/IterableIterator.h:71:51: error: too few arguments to function call, expected 3, have 1
>  4:13.31   return ToJSValue(aCx, ((*aInst).*aMethod)(aIndex), aResult);
>  4:13.31                          ~~~~~~~~~~~~~~~~~        ^
>  4:13.31 /home/kris/code/mozilla-central/obj-package/dom/bindings/MozSharedMapBinding.cpp:475:10: note: in instantiation of function template specialization 'mozilla::dom::CallIterableGetter<mozilla::dom::ipc::SharedMap, bool (JSContext *, unsigned int, JS::MutableHandle<JS::Value>) const>' requested here
>  4:13.31     if (!CallIterableGetter(cx, GetValueAtIndex, self, i,
>  4:13.31          ^
> 
> Which seems to be because I used `const T* aInst` for the const method version, and we pass a non-const pointer. I guess I could stick with a plain `T*`, but I generally prefer maintaining const-correctness to avoiding SFINAE. Or I could just add explicit const and non-const versions of the first method, too...
> 
> I changed the 3-argument forms to use what you suggested, though.

Upon further consideration... Just having explicit const and non-const versions of both forms, and using `U (T::* aMethod)(uint32_t)` for the first one is probably best. It's at least more consistent.

	Kris Maglione [:kmag] Assignee
	Comment 10 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0183c5a9b7ebde63bea5ed4aac778c3f46fb46ec
Bug 1472342: Part 1 - Properly support iterator getters which need JSContexts. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bba6a742743161357b207fed420b4311214d7cd
Bug 1472342: Part 2 - Update SharedMap iterator methods to use provided JSContext. r=bz

	Raul Gurzau (:RaulG)
	Comment 11 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0183c5a9b7eb
https://hg.mozilla.org/mozilla-central/rev/3bba6a742743