1390036 - Remove nsXPIDLString

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

This is about removing nsXPIDLString. nsXPIDLCString will be removed in a different bug.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove most remaining uses of nsXPIDLString

CompareCacheHashEntry::mCrit[] is the only case where the nsXPIDLString-ness
was important. The patch adds an explicit SetIsVoid() call to that class's
constructor and changes some null checks to IsVoid() checks.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove nsXPIDLString use from NullString()

This requires adding a new constructor for ns[C]String that creates an IsVoid
string.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove nsXPIDLString

Note that nsXPIDLCString still exists. I will remove that later.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove nsXPIDLString use from NullString()

I forgot to mark the constructor |explicit| in this patch.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove nsXPIDLString

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8896886 [details] [diff] [review]
(part 2) - Remove nsXPIDLString use from NullString()

Review of attachment 8896886 [details] [diff] [review]:
-----------------------------------------------------------------

David, do you have an opinion here?

::: xpcom/string/nsTString.h
@@ +33,5 @@
>      : substring_type(ClassFlags::NULL_TERMINATED)
>    {
>    }
>  
> +  explicit nsTString_CharT(bool aIsVoid)

I think I'd prefer just making |NullString| a friend of |nsTString_CharT| so that it can call the protected |nsTString_CharT(char_type*, size_type, DataFlags, ClassFlags)| constructor directly.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8896886 [details] [diff] [review]
(part 2) - Remove nsXPIDLString use from NullString()

I think I prefer the approach of exposing a new constructor on nsTString, but I think I'd prefer:
 (a) making that new constructor protected, and
 (b) probably exposing DataFlags as the type rather than boolean

(randomly setting the feedback? flag to something...)

Comment 8

[Eric Rahm [:erahm]]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #7)
> Comment on attachment 8896886 [details] [diff] [review]
> (part 2) - Remove nsXPIDLString use from NullString()
> 
> I think I prefer the approach of exposing a new constructor on nsTString,
> but I think I'd prefer:
>  (a) making that new constructor protected, and
>  (b) probably exposing DataFlags as the type rather than boolean
> 
> (randomly setting the feedback? flag to something...)

This sounds good, my main concern was the ctor not being protected.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove nsXPIDLString use from NullString()

<none>

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8897246 [details] [diff] [review]
(part 2) - Remove nsXPIDLString use from NullString()

>+  friend const nsString& NullString();

Please put this line in an
#ifdef CharT_is_PRUnichar

>+  explicit nsTString_CharT(DataFlags aDataFlags)
>+    : substring_type(ClassFlags::NULL_TERMINATED)
>+  {
>+    mDataFlags |= aDataFlags;

Instead of putting the mDataFlags manipulation in the body, could you call the other constructor:
    : substring_type(char_traits::sEmptyBuffer, 0,
                     aDataFlags, ClassFlags::NULL_TERMINATED)
since this might, in the future, help with assertions that happen in that constructor?


r=dbaron with that

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Or, on the other hand, maybe this should be adjusting NullCString as well, so that we can also remove nsXPIDLCString?)

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #10)
> Comment on attachment 8897246 [details] [diff] [review]
> (part 2) - Remove nsXPIDLString use from NullString()
> 
> >+  friend const nsString& NullString();
> 
> Please put this line in an
> #ifdef CharT_is_PRUnichar

I guess given (a) comment 0 and (b) that there's a NullCString function, it's probably best to add a macro for the name of the NullTString function to the "template" defines, and use that.

Comment 13

[Nicholas Nethercote [inactive]]

> Instead of putting the mDataFlags manipulation in the body, could you call
> the other constructor:
>     : substring_type(char_traits::sEmptyBuffer, 0,
>                      aDataFlags, ClassFlags::NULL_TERMINATED)
> since this might, in the future, help with assertions that happen in that
> constructor?

Sure, though I will do `aDataFlags | DataFlags::TERMINATED` instead of just `aDataFlags`.

Comment 14

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd7a1aa5db733489256e531e4e6c8bac9b4aae98
Bug 1390036 (part 1) - Remove most remaining uses of nsXPIDLString. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6de75921f1a41b02fbd2358edf3b23457105c9
Bug 1390036 (part 2) - Remove nsXPIDLString use from NullString(). r=dbaron.

https://hg.mozilla.org/integration/mozilla-inbound/rev/30b4593d421308ef2ba9d608732cfcf707c77b55
Bug 1390036 (part 3) - Remove nsXPIDLString. r=erahm.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd7a1aa5db73
https://hg.mozilla.org/mozilla-central/rev/5e6de75921f1
https://hg.mozilla.org/mozilla-central/rev/30b4593d4213