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

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.

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

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

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

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

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

<none>

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

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

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

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

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.

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