931571 - Make nsDiscriminatedUnion use methods

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: WIP patch stack smushed together

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.

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Use a constructor for nsDiscriminatedUnion.

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).

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Convert Cleanup() and FreeArray() to methods.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Rename return value of macro generated methods.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Fix some spacing inside macros.

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

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Turn basic ConvertTo functions into methods.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 6 - Make ConvertToID and String2ID into methods.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 7 - Turn ConvertTo*String and ToString into methods.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 8 - Turn ConvertToISupports, ConvertToInterface and ConvertToArray to methods.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 9 - Turn most of the nsVariant setters into nsDiscriminated methods.

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

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: part 10 - Turn SetFromVariant into a method.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: part 11 - Turn nsDiscriminatedUnion's Traverse function into a method.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: part 12 - Add nsDiscriminatedUnion::GetType().

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

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: part 13 - Add a destructor for nsDiscriminatedUnion.

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.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: part 14 - Delete various ways to copy or move nsDiscriminatedUnion.

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 18

[Nathan Froyd [:froydnj]]

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 19

[Nathan Froyd [:froydnj]]

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?

Comment 20

[Andrew McCreight [:mccr8]]

(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.

Comment 21

[Nathan Froyd [:froydnj]]

(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.)

Comment 22

[Andrew McCreight [:mccr8]]

(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 23

[Nathan Froyd [:froydnj]]

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 24

[Nathan Froyd [:froydnj]]

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...

Comment 25

[Andrew McCreight [:mccr8]]

(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".

Comment 26

[Nathan Froyd [:froydnj]]

(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

Comment 27

[Andrew McCreight [:mccr8]]

(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.

Comment 28

[Andrew McCreight [:mccr8]]

(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.

Comment 29

[Andrew McCreight [:mccr8]]

Attachment: part 15 - Remove extra spaces in nsVariant macros.

Comment 30

[Andrew McCreight [:mccr8]]

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

Comment 31

[Pulsebot]

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

Comment 32

[Carsten Book [:Tomcat]]

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