Remove nsXPIDLString

RESOLVED FIXED in Firefox 57

Status

()

Core
String
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

8 months ago
This is about removing nsXPIDLString. nsXPIDLCString will be removed in a different bug.
(Assignee)

Comment 1

8 months ago
Created attachment 8896869 [details] [diff] [review]
(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.
Attachment #8896869 - Flags: review?(erahm)
(Assignee)

Updated

8 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 months ago
Created attachment 8896870 [details] [diff] [review]
(part 2) - Remove nsXPIDLString use from NullString()

This requires adding a new constructor for ns[C]String that creates an IsVoid
string.
Attachment #8896870 - Flags: review?(erahm)
(Assignee)

Comment 3

8 months ago
Created attachment 8896872 [details] [diff] [review]
(part 3) - Remove nsXPIDLString

Note that nsXPIDLCString still exists. I will remove that later.
Attachment #8896872 - Flags: review?(erahm)
(Assignee)

Comment 4

8 months ago
Created attachment 8896886 [details] [diff] [review]
(part 2) - Remove nsXPIDLString use from NullString()

I forgot to mark the constructor |explicit| in this patch.
Attachment #8896886 - Flags: review?(erahm)
(Assignee)

Updated

8 months ago
Attachment #8896870 - Attachment is obsolete: true
Attachment #8896870 - Flags: review?(erahm)
(Assignee)

Comment 5

8 months ago
Created attachment 8896887 [details] [diff] [review]
(part 3) - Remove nsXPIDLString
Attachment #8896887 - Flags: review?(erahm)
(Assignee)

Updated

8 months ago
Attachment #8896872 - Attachment is obsolete: true
Attachment #8896872 - Flags: review?(erahm)
Attachment #8896869 - Flags: review?(erahm) → review+
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.
Attachment #8896886 - Flags: review?(erahm) → feedback?(dbaron)
Attachment #8896887 - Flags: review?(erahm) → review+
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...)
Attachment #8896886 - Flags: feedback?(dbaron) → feedback+
(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.
(Assignee)

Comment 9

8 months ago
Created attachment 8897246 [details] [diff] [review]
(part 2) - Remove nsXPIDLString use from NullString()

<none>
Attachment #8897246 - Flags: review?(dbaron)
(Assignee)

Updated

8 months ago
Attachment #8896886 - Attachment is obsolete: true
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
Attachment #8897246 - Flags: review?(dbaron) → review+
(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.
(Assignee)

Comment 13

8 months ago
> 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 15

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd7a1aa5db73
https://hg.mozilla.org/mozilla-central/rev/5e6de75921f1
https://hg.mozilla.org/mozilla-central/rev/30b4593d4213
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.