1371846 - Add IPC support for mozilla::Variant

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[Botond Ballo [:botond]]

mozilla::Variant does not currently support being sent over IPC.

It would be nice to add support for this. It would involve writing a ParamTraits specialization for Variant, similar to the one for Maybe [1].

[1] http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/ipc/glue/IPCMessageUtils.h#841

Comment 1

[Botond Ballo [:botond]]

This might be a good first bug for someone who is new to the Mozilla codebase, but has previous experience with C++. Please take that into account when volunteering :)

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

This would be super nice since Variant is nicer to work with than ipdl union types.

Comment 3

[Jeff Hajewski]

I am interested in taking this on. Other than adding the ParamTraits specialization and writing some tests, are there any other key steps?

Comment 4

[Botond Ballo [:botond]]

(In reply to Jeff Hajewski from comment #3)
> I am interested in taking this on. 

Thanks for your interest!

> Other than adding the ParamTraits
> specialization and writing some tests, are there any other key steps?

That should be it.

I haven't written tests specifically for IPC serialization before, but I agree that writing some for Variant might be a good idea. I took a quick look and it looks like the IPDL unit tests in ipc/ipdl/test/cxx would be a good fit for this purpose. See the README.txt in that directory [1] for instructions.

Let me know if you need any guidance about implementing the ParamTraits specialization, or writing tests, and feel free to ask if you have any questions.

[1] http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/ipc/ipdl/test/cxx/README.txt

Comment 5

[Jeff Hajewski]

(In reply to Botond Ballo [:botond] from comment #4)
> (In reply to Jeff Hajewski from comment #3)
> > I am interested in taking this on. 
> 
> Thanks for your interest!
> 
> > Other than adding the ParamTraits
> > specialization and writing some tests, are there any other key steps?
> 
> That should be it.
> 
> I haven't written tests specifically for IPC serialization before, but I
> agree that writing some for Variant might be a good idea. I took a quick
> look and it looks like the IPDL unit tests in ipc/ipdl/test/cxx would be a
> good fit for this purpose. See the README.txt in that directory [1] for
> instructions.
> 
> Let me know if you need any guidance about implementing the ParamTraits
> specialization, or writing tests, and feel free to ask if you have any
> questions.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> d840ebd5858a61dbc1622487c1fab74ecf235e03/ipc/ipdl/test/cxx/README.txt

Thanks for pointing me in the right direction of the IPDL unit tests! I did a little looking around for these yesterday but am still getting familiar with the code base.

Comment 6

[Jeff Hajewski]

I've been thinking about the best approach for writing the matcher used when implementing ParamTraits<Variant>::Write(msg,param) and struggling with how the type inference will work.

The matcher struct will contain a templated match method, which will contain two WriteParam() calls, one to write the tag and one to write the actual value being stored by the variant. What I'm not sure about is how to template the matcher struct. The tricky part is figuring out the tag value. It is not publicly exposed via Variant<>, and even if it were, match doesn't actually have a reference to the calling variant, just the extracted value. The idea then would be for the matcher to somehow look up the tag for a given type in its given type pack.

My initial thought is something like this:

template<typename... Ts>
struct IPCMatcher
{
    // Can the compiler infer the type T here?
    // It seems like it should be able to, but I am
    // still experimenting with this
    template <typename T>
    void match(T& t) {
        auto tag = detail::SelectVariantType<T, Ts...>::count;
        WriteParam(msg, tag);
        WriteParam(msg, t);
    }
};

I'm not sure if that is valid, specifically with respect to the second template on T. I'm playing around with a small trivial class to see if this works, but figured I'd also post here in case I'm moving in the wrong direction. This was the cleanest way I could think of to get the tag value.

As for the ParamTraits<Variant>::Read() method, we have the type pack Ts... and the tag, which should correspond to the type position in the type pack. What I'm struggling with (although I haven't thought through this as much as the write) is why can't we simply use Nth<tag, Ts...>::Type as the type? Then we wouldn't need to add any additional functionality an we can write something along the lines of:

static bool Read(const Message* msg, PickleIterator* iter, paramType* result)
{
    size_t tag;
    if (!ReadParam(msg, iter, &tag)) {
        return false;
    }

    Nth<tag, Ts...>::Type val; // Not sure if this is valid...
    if (!ReadParam(msg, iter, &val) {
        return false;
    }
    *result = val;
    return true;
}

I'll keep thinking about this, but before I spin my wheels for too long I figured I better check-in and make sure I'm on the right track.

Comment 7

[Botond Ballo [:botond]]

Great questions, Jeff, you're definitely on the right track!

(In reply to Jeff Hajewski from comment #6)
> The matcher struct will contain a templated match method, which will contain
> two WriteParam() calls, one to write the tag and one to write the actual
> value being stored by the variant. 

It would be simpler to have the WriteParam() call for the tag directly in ParamTraits::Write(), rather than inside the matcher, since it doesn't depend on the type of the element stored inside the variant. This avoids having to template the matcher on the entire variant type, and passing in a reference to the variant to the matcher.

> The tricky part is figuring out the tag value.
> It is not publicly exposed via Variant<>, 

Good point, I didn't realize that. However, it's straightforward to fix: Variant can declare the ParamTraits specialization for it as a friend, giving it access to private/protected members. We do this already for other types we serialize (see [1] for an example).

> and even if it were, match doesn't
> actually have a reference to the calling variant, just the extracted value.

We could pass in a reference to the calling variant when constructing the matcher. Not having to do that is the reason writing the tag directly from ParamTraits::Write() is simpler :)

> template<typename... Ts>
> struct IPCMatcher
> {
>     // Can the compiler infer the type T here?
>     // It seems like it should be able to, but I am
>     // still experimenting with this
>     template <typename T>
>     void match(T& t) {

Yep, this part is fine. Variant::match() will call this function with the correct element type as argument, allowing the compiler to deduce the type "T".

> As for the ParamTraits<Variant>::Read() method, we have the type pack Ts...
> and the tag, which should correspond to the type position in the type pack.
> What I'm struggling with (although I haven't thought through this as much as
> the write) is why can't we simply use Nth<tag, Ts...>::Type as the type?
> Then we wouldn't need to add any additional functionality an we can write
> something along the lines of:
> 
> static bool Read(const Message* msg, PickleIterator* iter, paramType* result)
> {
>     size_t tag;
>     if (!ReadParam(msg, iter, &tag)) {
>         return false;
>     }
> 
>     Nth<tag, Ts...>::Type val; // Not sure if this is valid...
>     if (!ReadParam(msg, iter, &val) {
>         return false;
>     }
>     *result = val;
>     return true;
> }

It would be nice if this worked, as it's nice and simple, but it doesn't :) Template instantiation happens at compile time, which means that template arguments have to be resolved at compile time. In the case of a non-type template parameter like the "size_t N" parameter of "Nth", its value needs to be known at compile time. However, "tag" here is a value that's only known at runtime (and that's necessarily the case, since the purpose of Variant is to allow making a runtime choice between different alternative types to store).

To make this work, we need to have code for each case, and choose among them at runtime. If the number of elements were fixed (say, 3), it could look like this:

  if (tag == 0) {
    Nth<0, Ts...>::Type val;
    if (!ReadParam(msg, iter, &val)) {
      return false;
    }
    *result = val;
  } else if (tag == 1) {
    Nth<1, Ts...>::Type val;
    if (!ReadParam(msg, iter, &val)) {
      return false;
    }
    *result = val;
  } else if (tag == 2) {
    Nth<2, Ts...>::Type val;
    if (!ReadParam(msg, iter, &val)) {
      return false;
    }
    *result = val;
  }

Notice that here, we have a separate piece of code for each element. In each case, the argument provided to "Nth" is known at compile time ("0", "1", "2", as opposed to "tag"), and we then use the value of "tag" to choose which branch we run at runtime.

Since the number of elements isn't fixed, we need some template-based recursion to generate code like this for each possible tag value "I" from 0 to (N - 1), where N is the number of elements in the variant. I won't go into detail about that now because I've written a lot already, but there are several examples of such recursion in Variant.h, like "VariantImplementation" (even "Nth" is a simple example of compile-time recursion). I'm happy to go into more detail later if you get stuck on this part.

[1] http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/gfx/layers/FrameMetrics.h#45

Comment 8

[Jeff Hajewski]

I've been working on this over the weekend and while I understand the mechanics of how it should work, I could use some input on how to best design the actual implementation. Specifically, the two approaches I have thought of are either creating a helper class and then adding an instance of this to ParamTraits or making Variant a friend of ParamTraits and implementing the read in Variant. I don't think either of these is a great solution. Making ParamTraits a friend and Variant a friend of each other seems to couple these two classes together, which I would think we would want to avoid as much as possible (if everyone is adding a small piece of code that couples two components, eventually we will have a mess). Implementing the Read method in Variant doesn't seem like the right move either because that functionality isn't a defining characteristic of Variant, so I think that logic should be separate from the Variant class.

So anyways, here's where I'm at. The implementation would look something like:

template <size_t N, typename... Ts>
struct SomeClassYetToBeDetermined<N, Ts...> {
    // We choose the base case to be SomeClassToBeDetermined<0, Ts...>
    using Next = SomeClassYetToBeDetermined<N-1, Ts...>;

    static void Read(Tag t) {
        if (t == N) {
            // This part can be done in one of two ways I think...
            // 1. We could just use Nth<N>::Type, like previously discussed
            // 2. We can peel off T from Ts... each time we go to Next::
            //    Although this approach would require we use N as our base case
            //
            // This leaves us with either:
            // Nth<N>::Type val
            //      OR
            // T val;
            
            T val; // for brevity
            ReadParam(msg, iter, &val); // leaving out some details here..
        } else {
            Next::Read(t);
        }
};

Of course we will also need a specialization for the base case where the else branch has a static_assert, but I will leave that out for now for the sake of space.

Now that I'm getting my thoughts down in writing, I'm thinking making a Helper class might be the right approach and only add it to the Variant specialized ParamTraits specialization (I need to double check that adding it to a single specialization is legal...). Something like:

template <size_t N, typename... Ts>
VariantIpcHelper<N, T> {
    Read(Tag t) {
        if (t == N) {
            // OK -- read in data
        } else {
            // Error -- static_assert here
        }
    }
};

template <size_t N, typename... Ts>
VariantIpcHelper<N, T, Ts...> {
    using Next = VariantIpcHelper<N+1, Ts...>;

    Read(Tag t) {
        if (t == N) {
            // OK - read in data
        } else {
            Next::Read(t);
        }
    }
};

template <typename... Ts>
struct ParamTraits<Variant<Ts...>> {
    using Helper = VariantIpcHelper<0, Ts...>;

    // Rest of implementation
};

Of course, this has the same issue I mentioned earlier of having to link these so that VariantIpcHelper can call the ParamTraits methods (e.g., ReadParam). I guess the core issue is that we need some way to kick off the recursive template definition (which we do here with using Helper = VariantIpcHelper<0, Ts...>) and it should not be the user's responsibility to take care of this. Using a helper class seems to overcome this hurdle, but adds in its own issues... What are your thoughts Botond?

Comment 9

[Jeff Hajewski]

I've been thinking about this some more, and I think my last approach might be on the right track. Since ReadParam() is a static method, I think something along these lines should work...

template <size_t N, typename... Ts>
VariantIpcHelper<N, T> {
    static Variant<Ts...> Read(const Message* msg, PickleIterator* iter, Tag t) {
        if (t == N) {
            T val;
            ParamTraits::ReadParam(msg, iter, &val);
            return AsVariant(val)
        } else {
            // Error -- static_assert here
        }
    }
};

template <size_t N, typename... Ts>
VariantIpcHelper<N, T, Ts...> {
    using Next = VariantIpcHelper<N+1, Ts...>;

    static Variant<Ts...> Read(const Message* msg, PickleIterator* iter, Tag t) {
        if (t == N) {
            T val;
            ParamTraits::ReadParam(msg, iter, &val);

            // I think this should work since we've specified the return
            // as Variant<Ts...>
            return AsVariant(val);
        } else {
            return Next::Read(t);
        }
    }
};

template <typename... Ts>
struct ParamTraits<Variant<Ts...>> {
    using Helper = VariantIpcHelper<0, Ts...>;

    static bool Read(const Message* msg, PickleIterator* iter, paramType* result) {
        Tag t;
        if (!ReadParam(msg, iter, &tag)) {
            return false;
        }
        *result = Helper::Read(msg, iter, t);
        return true;
    }
};

This is a rough sketch -- I'm sure there are a few details that are incorrect here but I think it's going in the right direction. I need to think a little bit more about handling failure in ParamTraits::Read(). On the one hand I'm thinking if we read in a tag then we should be guaranteed to be able to find the correct type. On the other hand, nothing is ever really guaranteed is it?

Comment 10

[Botond Ballo [:botond]]

(In reply to Jeff Hajewski from comment #8)
> the two approaches I have
> thought of are either creating a helper class and then adding an instance of
> this to ParamTraits or making Variant a friend of ParamTraits and
> implementing the read in Variant. 

I envisioned using a helper class to implement the recursion for Read().

> Making ParamTraits a friend and Variant a friend of each other
> seems to couple these two classes together, which I would think we would
> want to avoid as much as possible (if everyone is adding a small piece of
> code that couples two components, eventually we will have a mess).
> Implementing the Read method in Variant doesn't seem like the right move
> either because that functionality isn't a defining characteristic of
> Variant, so I think that logic should be separate from the Variant class.

Agreed.

> Now that I'm getting my thoughts down in writing, I'm thinking making a
> Helper class might be the right approach and only add it to the Variant
> specialized ParamTraits specialization (I need to double check that adding
> it to a single specialization is legal...). 

Yep, that's fine.

One thing you might find helpful, is to define the helper class as a nested class inside the ParamTraits<Variant<Ts...>> specialization. Then, depending on how exactly you implement the recursion, you may not need to template the helper class on "Ts...", since the "Ts..." from the enclosing class will already be in scope.

(In reply to Jeff Hajewski from comment #9)
> I need to
> think a little bit more about handling failure in ParamTraits::Read(). On
> the one hand I'm thinking if we read in a tag then we should be guaranteed
> to be able to find the correct type. On the other hand, nothing is ever
> really guaranteed is it?

We should definitely propagate the return value of the ReadParam() call we make inside the helper class, to the return value of ParamTraits::Read(). One way to do this would be to use the return value of the helper class's Read() method to communicate success/failure (currently we use it to pass back the resulting Variant, but we could do that another way, such as by passing a reference to it as an argument).

Anyways, based on the code in comment 9, you're definitely on the right track, and it looks like you're close to getting things working!

Comment 11

[Jeff Hajewski]

(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to Jeff Hajewski from comment #8)
> > the two approaches I have
> > thought of are either creating a helper class and then adding an instance of
> > this to ParamTraits or making Variant a friend of ParamTraits and
> > implementing the read in Variant. 
> 
> I envisioned using a helper class to implement the recursion for Read().
> 
> > Making ParamTraits a friend and Variant a friend of each other
> > seems to couple these two classes together, which I would think we would
> > want to avoid as much as possible (if everyone is adding a small piece of
> > code that couples two components, eventually we will have a mess).
> > Implementing the Read method in Variant doesn't seem like the right move
> > either because that functionality isn't a defining characteristic of
> > Variant, so I think that logic should be separate from the Variant class.
> 
> Agreed.
> 
> > Now that I'm getting my thoughts down in writing, I'm thinking making a
> > Helper class might be the right approach and only add it to the Variant
> > specialized ParamTraits specialization (I need to double check that adding
> > it to a single specialization is legal...). 
> 
> Yep, that's fine.
> 
> One thing you might find helpful, is to define the helper class as a nested
> class inside the ParamTraits<Variant<Ts...>> specialization. Then, depending
> on how exactly you implement the recursion, you may not need to template the
> helper class on "Ts...", since the "Ts..." from the enclosing class will
> already be in scope.

I've been giving this some thought, it seems one way or another we will need to recurse on the <Ts...> list. If we don't do it in the helper class, we will need to do it in the ParamTraits specialization. Assuming the above is correct, then it's a matter of do we want N copies of the helper class for each ParamTraits specialization, or do we want N copies of each ParamTraits specilization (and move the Next value to the ParamTraits specialization).

What are your thoughts? I may just need to think about this some more..

Comment 12

[Botond Ballo [:botond]]

(In reply to Jeff Hajewski from comment #11)
> > One thing you might find helpful, is to define the helper class as a nested
> > class inside the ParamTraits<Variant<Ts...>> specialization. Then, depending
> > on how exactly you implement the recursion, you may not need to template the
> > helper class on "Ts...", since the "Ts..." from the enclosing class will
> > already be in scope.
> 
> Ive been giving this some thought, it seems one way or another we will need
> to recurse on the <Ts...> list. If we don't do it in the helper class, we
> will need to do it in the ParamTraits specialization. Assuming the above is
> correct, then it's a matter of do we want N copies of the helper class for
> each ParamTraits specialization, or do we want N copies of each ParamTraits
> specilization (and move the Next value to the ParamTraits specialization).
> 
> What are your thoughts? I may just need to think about this some more..

I don't think that serialization of a type T should involve any ParamTraits specialization besides ParamTraits<T>.

The helper class will definitely need to be templated, as we will need a different instantiation for each element of the variant. However, it may be sufficient to template it on the index N only, if the complete element list <Ts...> of the variant type is in scope (from the enclosing ParamTraits specialization), and we use Nth<N, Ts...> to access the Nth element in that list.

Alternatively, we could template the helper class on <Ts...>, and use the recursion to access each element in turn instead of using Nth. In this case, there is no need to access the <Ts...> of the ParamTraits specialization, and therefore the helper class can be placed at namespace scope rather than being nested inside ParamTraits.

Let me know if that makes sense.

Comment 13

[Jeff Hajewski]

(In reply to Botond Ballo [:botond] from comment #12)
> > 
> > What are your thoughts? I may just need to think about this some more..
> 
> I don't think that serialization of a type T should involve any ParamTraits
> specialization besides ParamTraits<T>.
> 
> The helper class will definitely need to be templated, as we will need a
> different instantiation for each element of the variant. However, it may be
> sufficient to template it on the index N only, if the complete element list
> <Ts...> of the variant type is in scope (from the enclosing ParamTraits
> specialization), and we use Nth<N, Ts...> to access the Nth element in that
> list.
> 
> Alternatively, we could template the helper class on <Ts...>, and use the
> recursion to access each element in turn instead of using Nth. In this case,
> there is no need to access the <Ts...> of the ParamTraits specialization,
> and therefore the helper class can be placed at namespace scope rather than
> being nested inside ParamTraits.
> 
> Let me know if that makes sense.

That makes perfect sense -- I was getting too focused on the recursion that I forgot about Nth.

Comment 14

[Jeff Hajewski]

Attachment: Add IPC support for mozilla::Variant (bug 1371846);

Changes made:
* Add IPC::ParamTraits as a friend to mozilla::Variant in Variant.h.
  This is required so that `tag` can be accessed in the
  IPC::ParamTraits specialization.

* Add a IPC::ParamTraits specialization to IPCMessageUtils.h.

Review commit: https://reviewboard.mozilla.org/r/159804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159804/

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8888757 [details]
Add IPC support for mozilla::Variant (bug 1371846);

https://reviewboard.mozilla.org/r/159804/#review165210

Jeff, this looks great!

I just have a few minor comments.

There is a bit of code duplication between the bodies of IPCVariantHelper<N>::Read() and IPCVariantHelper<0>::Read() that would be nice to avoid. On a related note, notice that, for a variant with N elements, the tag will be between 0 and N-1, but the code will instantiate IPCVariantHelper<N> through IPCVariantHelper<0>; the IPCVariantHelper<N> instantiation is unnecessary, as |t == N| will never be true for it. We could use this observation to eliminate the code duplication in the following way: we could have IPCVariantHelper<N> check for |t == (N-1)| instead of |t == N|. That would make the IPCVariantHelper<0> instantiation the one that will never match an element, so we can just unconditionally return false form IPCVariantHelper<0>::Read(). Let me know if that makes sense.

::: ipc/glue/IPCMessageUtils.h:912
(Diff revision 1)
> +struct ParamTraits<mozilla::Variant<Ts...>>
> +{
> +  typedef mozilla::Variant<Ts...> paramType;
> +  using Tag = typename mozilla::detail::VariantTag<Ts...>::Type;
> +
> +  struct IPCVariantMatcher

Since this matcher is specific to the Write() operation, let's call it IPCVariantWriteMatcher, or just IPCVariantWriter.

Actually, the "IPC" prefix can be dropped too - we're inside IPC code so it's kind of implied.

::: ipc/glue/IPCMessageUtils.h:923
(Diff revision 1)
> +      WriteParam(msg, t);
> +    }
> +  };
> +
> +  // IPCVariantHelper is used to get the type for a given tag
> +  template<size_t N, typename dummy = void>

Presumably the reason you need a |dummy| parameter is that without it, the N = 0 specialization would be an explicit specialization (as opposed to a partial specialization), and you can't explicitly specialize a nested class template without also explicitly specializing its enclosing class template.

Assuming that's correct, it might be useful to mention that in a comment, in case someone reading the code is wondering why that |dummy| parameter is there.

::: ipc/glue/IPCMessageUtils.h:924
(Diff revision 1)
> +    }
> +  };
> +
> +  // IPCVariantHelper is used to get the type for a given tag
> +  template<size_t N, typename dummy = void>
> +  struct IPCVariantHelper

Similarly to IPCVariantHelper, as this is for the Read() operation, let's call it IPCVariantReadHelper or just IPCVariantReader (and again the IPC prefix can be dropped as well).

::: ipc/glue/IPCMessageUtils.h:929
(Diff revision 1)
> +  struct IPCVariantHelper
> +  {
> +    using Next = IPCVariantHelper<N-1>;
> +
> +    static bool Read(const Message* msg, PickleIterator* iter,
> +        Tag t, paramType* result)

Let's name the parameter "tag" instead of "t".

::: ipc/glue/IPCMessageUtils.h:934
(Diff revision 1)
> +        Tag t, paramType* result)
> +    {
> +      if (t == N) {
> +        typename mozilla::detail::Nth<N, Ts...>::Type val;
> +        if (ReadParam(msg, iter, &val)) {
> +          *result = mozilla::Variant<Ts...>::AsVariant(val);

We can use |paramType| instead of |mozilla::Variant<Ts...>| here.

::: ipc/glue/IPCMessageUtils.h:954
(Diff revision 1)
> +        Tag t, paramType* result)
> +    {
> +      if (t == 0) {
> +        typename mozilla::detail::Nth<0, Ts...>::Type val;
> +        if (ReadParam(msg, iter, &val)) {
> +          *result = mozilla::Variant<Ts...>::AsVariant(val);

Likewise.

::: ipc/glue/IPCMessageUtils.h:973
(Diff revision 1)
> +      return IPCVariantHelper<sizeof...(Ts)>::Read(msg, iter, tag, result);
> +    }
> +    return false;
> +  }
> +
> +  static void Write(Message* msg, const paramType& param)

I would move this up to be below IPCVariantMatcher. That way the write helper is close to the write function, and the read helper is close to the read function.

Comment 16

[Jeff Hajewski]

(In reply to Botond Ballo [:botond] from comment #15)

Thanks for the feedback and great comments! I can't believe I missed the N vs. N - 1 point... anyways, I've made the fixes and have one question (below). Should I make a new commit and push to reviewboard again?

> Comment on attachment 8888757 [details]
> Add IPC support for mozilla::Variant (bug 1371846);
> 
> https://reviewboard.mozilla.org/r/159804/#review165210
> 
> Jeff, this looks great!
> 
> I just have a few minor comments.
> 
> There is a bit of code duplication between the bodies of
> IPCVariantHelper<N>::Read() and IPCVariantHelper<0>::Read() that would be
> nice to avoid. On a related note, notice that, for a variant with N
> elements, the tag will be between 0 and N-1, but the code will instantiate
> IPCVariantHelper<N> through IPCVariantHelper<0>; the IPCVariantHelper<N>
> instantiation is unnecessary, as |t == N| will never be true for it. We
> could use this observation to eliminate the code duplication in the
> following way: we could have IPCVariantHelper<N> check for |t == (N-1)|
> instead of |t == N|. That would make the IPCVariantHelper<0> instantiation
> the one that will never match an element, so we can just unconditionally
> return false form IPCVariantHelper<0>::Read(). Let me know if that makes
> sense.
> 

Do we want to compare t with N - 1 here or should we just initialize the template with sizeof...(Ts) - 1? On the one hand, comparing t with N - 1 makes the implementation simple since we can just return false on the N = 0 specialization. However, it might be confusing seeing the N - 2 and N - 1s in the implementation. I suppose I could also just add a comment explaining that part. What are your thoughts?

Comment 17

[Botond Ballo [:botond]]

(In reply to Jeff Hajewski from comment #16)
> Should I make a new commit and push to reviewboard again?

Please use "hg commit --amend" to fold your changes into the existing 
commit, and push to reviewboard again.

> Do we want to compare t with N - 1 here or should we just initialize the
> template with sizeof...(Ts) - 1? On the one hand, comparing t with N - 1
> makes the implementation simple since we can just return false on the N = 0
> specialization. However, it might be confusing seeing the N - 2 and N - 1s
> in the implementation. 

I would prefer to avoid the code duplication one way or another.

If the off-by-one bothers you, another approach would be to start the
recursion at |sizeof...(Ts) - 1|, and make the base case be -1 instead
of 0 (which can then just return false). (This would require changing
the type of the template parameter from |size_t| to |int|.)

> I suppose I could also just add a comment explaining
> that part.

A comment would definitely be helpful if we go with the off-by-one
approach.

Comment 18

[Jeff Hajewski]

> (This would require changing the type of the template parameter from |size_t| to |int|.)

Great point, I did not consider about this. I think it is best to stick with the off-by-one approach. I will make a few small updates, amend the commit, and push to reviewboard this afternoon. Thanks!

Comment 19

[Jeff Hajewski]

Comment on attachment 8888757 [details]
Add IPC support for mozilla::Variant (bug 1371846);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159804/diff/1-2/

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8888757 [details]
Add IPC support for mozilla::Variant (bug 1371846);

https://reviewboard.mozilla.org/r/159804/#review165532

Thanks! Just a couple of final nits:

::: ipc/glue/IPCMessageUtils.h:929
(Diff revision 2)
> +    WriteParam(msg, param.tag);
> +    param.match(VariantWriter(msg));
> +  }
> +
> +  // Because `VariantReader` is a nested struct, we need the `dummy` template
> +  // parameter to avoid explicitly specializing the `VariantReader`

I would just say here: " ... to avoid making VariantReader<0> an explicit specialization, which is not allowed for a nested class template".

::: ipc/glue/IPCMessageUtils.h:941
(Diff revision 2)
> +
> +    static bool Read(const Message* msg, PickleIterator* iter,
> +        Tag tag, paramType* result)
> +    {
> +      // There are N tags starting at 0 and ending at N - 1.
> +      // Since we start at N (the size of the parameter pack Ts),

"Since the VariantReader specializations start at N"

::: ipc/glue/IPCMessageUtils.h:977
(Diff revision 2)
> +  };
> +
> +  static bool Read(const Message* msg, PickleIterator* iter, paramType* result)
> +  {
> +    Tag tag;
> +    if(ReadParam(msg, iter, &tag)) {

nit: there should be a space between the 'if' and the '('

Comment 21

[Jeff Hajewski]

Comment on attachment 8888757 [details]
Add IPC support for mozilla::Variant (bug 1371846);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159804/diff/2-3/

Comment 22

[Botond Ballo [:botond]]

Thanks!

I pushed the change to the Try server to make sure it's building on all platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35ef890f37a9

Comment 23

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #22)
> I pushed the change to the Try server to make sure it's building on all
> platforms:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=35ef890f37a9

That's looking good, so I went ahead and committed the patch via Autoland.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4322e8f2e455
Add IPC support for mozilla::Variant ; r=botond

Comment 25

[Jeff Hajewski]

(In reply to Botond Ballo [:botond] from comment #23)
> (In reply to Botond Ballo [:botond] from comment #22)
> > I pushed the change to the Try server to make sure it's building on all
> > platforms:
> > 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=35ef890f37a9
> 
> That's looking good, so I went ahead and committed the patch via Autoland.

Fantastic! Thanks again for all the help!

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4322e8f2e455