nsDiscriminated union has a bunch of methods, but instead of being on nsDiscriminatedUnion, they are static methods on nsVariant. The comment on nsDU added on the initial landing of this class in bug 44675 saying "It has no methods. So, its use requires no linkage to the xpcom module." Hopefully that's not still an issue, and we can make this a regular class with a constructor, methods, and private fields.

This does most of the conversion. There are some places in XPCVariant that reach into the class that maybe could be turned into a method on nsDiscriminatedUnion, in which case we could make the data fields private. It may also make sense to move nsDiscriminatedUnion into its own file. Also, the field name "u" is not the best, so maybe that should be changed.

I dusted off my patches for this. My apologies in advance to you and your review queue, Nathan.

Try run is green. There's no hurry on getting these reviewed, Nathan. There are a lot of patches, and they are not too small, but they consist mostly of a bunch of search and replace of nsVariant::Whatever(foo, bar) --> foo->Whatever(bar).

There's more that could be done to fix the style, but this was bothering me.

This also adds a new nsDiscriminatedUnion method SetFromDOMString, as somebody added an nsVariant method without the corresponding helper.

This is not quite enough to make the data members private because XPCVariant pokes around to do some JS array stuff.

The existing nsDiscriminateUnions either always call Cleanup() when they are about to go away, or they only handle scalar values so it is safe to call Cleanup() on them without worrying about another discriminated union having taken over any memory owned by this union.

nsDiscriminatedUnion owns memory without using smart pointers, so implementing anything that would copy or move around one of these will require some care. Just forbid these for now.

Comment on attachment 8635370 [details] [diff] [review] part 5 - Turn basic ConvertTo functions into methods. Review of attachment 8635370 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/nsVariant.cpp @@ +65,5 @@ > // This group results in a int32_t... > > #define CASE__NUMBER_INT32(type_, member_) \ > case nsIDataType :: type_ : \ > + aOutData->u.mInt32Value = u. member_ ; \ I don't think this space before |member_| is necessary... @@ +457,5 @@ > > /***************************************************************************/ > > +#define TRIVIAL_DATA_CONVERTER(type_, member_, retval_) \ > + if (mType == nsIDataType :: type_) { \ ...or the spaces here. If you wanted to fix them in passing (assuming it does compile, of course), I'd be OK with that.

Comment on attachment 8635372 [details] [diff] [review] part 7 - Turn ConvertTo*String and ToString into methods. Review of attachment 8635372 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCVariant.cpp @@ +735,5 @@ > > /* AString getAsAString (); */ > NS_IMETHODIMP XPCVariant::GetAsAString(nsAString & _retval) > { > + return mData.ConvertToAString(_retval); Want to file a followup bug on s/_retval/aRetval/g?

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #19) > Want to file a followup bug on s/_retval/aRetval/g? Yeah, it is pretty ugly, but this code is in XPConnect, and thus using JS style, and _retval feels a little more like JS style than aRetval.

(In reply to Andrew McCreight [:mccr8] from comment #20) > (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #19) > > Want to file a followup bug on s/_retval/aRetval/g? > > Yeah, it is pretty ugly, but this code is in XPConnect, and thus using JS > style, and _retval feels a little more like JS style than aRetval. Well, the standard says Don't Do That. :) ("That" being identifiers with leading underscores.)

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #21) > Well, the standard says Don't Do That. :) ("That" being identifiers with > leading underscores.) Oh I see. Yeah, retval_ would likely be better. Also, we use _retval, specifically, in an alarmingly large number of places. I guess it was the fashion of the time. http://mxr.mozilla.org/mozilla-central/ident?i=_retval&tree=mozilla-central&filter=

Comment on attachment 8635379 [details] [diff] [review] part 13 - Add a destructor for nsDiscriminatedUnion. Review of attachment 8635379 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/nsVariant.h @@ +31,5 @@ > { > public: > > nsDiscriminatedUnion() : mType(nsIDataType::VTYPE_EMPTY) {} > + ~nsDiscriminatedUnion() { Cleanup(); } Does this mean we could make Cleanup private?

Comment on attachment 8635380 [details] [diff] [review] part 14 - Delete various ways to copy or move nsDiscriminatedUnion. Review of attachment 8635380 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/nsVariant.h @@ +38,3 @@ > ~nsDiscriminatedUnion() { Cleanup(); } > > + nsDiscriminatedUnion& operator=(const nsDiscriminatedUnion&) = delete; No deletion of the move assignment operator? I'm pretty sure we don't want the compiler-generated one...

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #24) > No deletion of the move assignment operator? I'm pretty sure we don't want > the compiler-generated one... I just copied the list of stuff from a Wikpedia entry talking about "the rule of five".

(In reply to Andrew McCreight [:mccr8] from comment #25) > (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #24) > > No deletion of the move assignment operator? I'm pretty sure we don't want > > the compiler-generated one... > > I just copied the list of stuff from a Wikpedia entry talking about "the > rule of five". It looks like the move assignment operator is listed there? https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)#Rule_of_5

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #26) > It looks like the move assignment operator is listed there? Weird. I guess I forgot to add it, then when I went back to counting how many I had, I was incorrectly including the no-argument ctor as one of the five.

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #23) > Does this mean we could make Cleanup private? No, it is needed in XPCVariant's Unlink method.

Thanks for all of the reviews. I deleted the move assignment operator.

https://hg.mozilla.org/mozilla-central/rev/39c4a62c20a1 https://hg.mozilla.org/mozilla-central/rev/0d42e66ab791 https://hg.mozilla.org/mozilla-central/rev/68432d6b66dc https://hg.mozilla.org/mozilla-central/rev/80dbd90c5291 https://hg.mozilla.org/mozilla-central/rev/a0528bddd48c https://hg.mozilla.org/mozilla-central/rev/ed99b6a2c10e https://hg.mozilla.org/mozilla-central/rev/8d0d7f5851ff https://hg.mozilla.org/mozilla-central/rev/c015f6d6d6f8 https://hg.mozilla.org/mozilla-central/rev/22b849748747 https://hg.mozilla.org/mozilla-central/rev/26a36f6a5c36 https://hg.mozilla.org/mozilla-central/rev/91895b6135d1 https://hg.mozilla.org/mozilla-central/rev/c48aad69c171 https://hg.mozilla.org/mozilla-central/rev/9e87c9dec6f9 https://hg.mozilla.org/mozilla-central/rev/bbfac56ee224 https://hg.mozilla.org/mozilla-central/rev/e83d4dbc7dea

https://hg.mozilla.org/mozilla-central/rev/39c4a62c20a1 https://hg.mozilla.org/mozilla-central/rev/0d42e66ab791 https://hg.mozilla.org/mozilla-central/rev/68432d6b66dc https://hg.mozilla.org/mozilla-central/rev/80dbd90c5291 https://hg.mozilla.org/mozilla-central/rev/a0528bddd48c https://hg.mozilla.org/mozilla-central/rev/ed99b6a2c10e https://hg.mozilla.org/mozilla-central/rev/8d0d7f5851ff https://hg.mozilla.org/mozilla-central/rev/c015f6d6d6f8 https://hg.mozilla.org/mozilla-central/rev/22b849748747 https://hg.mozilla.org/mozilla-central/rev/26a36f6a5c36 https://hg.mozilla.org/mozilla-central/rev/91895b6135d1 https://hg.mozilla.org/mozilla-central/rev/c48aad69c171 https://hg.mozilla.org/mozilla-central/rev/9e87c9dec6f9 https://hg.mozilla.org/mozilla-central/rev/bbfac56ee224 https://hg.mozilla.org/mozilla-central/rev/e83d4dbc7dea