1346100 - Make nsTSubstringSplitter_CharT use nsTDependentSubstring_CharT for its pieces

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[(Away)]

Nathan suggested dependent substrings in bug 1330326 for the sake of avoiding allocations, but there's also a deeper reason: nsAString is meant to be abstract and never instantiated directly. (This was hinted at by the protected constructor that was worked around with the friend declaration.) The splitter violates this when it |new|s substrings into its array.

I noticed this in an earlier attempt at bug 1344629 when I (temporarily) added a pure virtual function to nsAString, to test its comment's claim that no one uses it directly. The substring splitter was the only thing that complained.

I took a different approach in 1344629 and I no longer _need_ the splitter to change, but it seems like a good idea to patch it anyway for the sake of cleanliness, and getting rid of that friend statement is a nice bonus.

Comment 1

[(Away)]

Attachment: patch

Bugzilla says Nathan isn't accepting reviews. I'll wait...

Comment 2

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

(In reply to David Major [:dmajor] from comment #0)
> Nathan suggested dependent substrings in bug 1330326 for the sake of
> avoiding allocations,

This is a good reason.

>  but there's also a deeper reason: nsAString is meant
> to be abstract and never instantiated directly. (This was hinted at by the
> protected constructor that was worked around with the friend declaration.)

I don't agree with this.  The nsAString that was meant to be abstract no longer exists; it's been merged into nsSubstring, and the name remains as a residual typedef from back when we needed to retain XPCOM binary compatibility with the old binary string interface.  nsSubstring and nsCSubstring are the preferred names (although it looks like we use the other names in the binary -- I'm not sure if that was for some sort of compatibility or if it was just what I'd call a mistake).


If we change the string API,  I'd actually prefer to evolve the string API into allowing more direct use of the base types, and limiting the things that are distinguish by API consumers using the type system to be character type (nsSubstring vs nsCSubstring, etc.) and null termination (nsSubstring vs nsString).  In most cases I don't think the ownership needs to be an aspect of the static type; the current code is mostly happy with it being dynamic, except for various methods on derived types (like nsDependentSubstring::Rebind) that don't really belong there.

Comment 3

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

Comment on attachment 8845728 [details] [diff] [review]
patch

It seems better to preserve the fact that the constructor is inlined into its single caller, Split.  I think you can do that by adding the "inline" keyword and then moving the function up one function in the .cpp file so that it's above Split.

r=dbaron

(though see my previous comment on your rationale; the patch is still good, though)

Comment 4

[(Away)]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #2)
> nsSubstring and
> nsCSubstring are the preferred names (although it looks like we use the
> other names in the binary -- I'm not sure if that was for some sort of
> compatibility or if it was just what I'd call a mistake).

On this topic, I happen to be doing some renaming over in bug 1346078. Would you like me to hold off on landing that and instead make nsSubstring become the canonical name in the binary?

Comment 5

[(Away)]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #3)
> It seems better to preserve the fact that the constructor is inlined into
> its single caller, Split.  I think you can do that by adding the "inline"
> keyword and then moving the function up one function in the .cpp file so
> that it's above Split.

I want to say I had to do this because nsTDependentSubstring wasn't fully defined at the point of the header. I'll give your suggestion a try, though.

(FWIW I haven't worried about cpp/h distinctions in years. LTCG/WPO are pretty amazing. :-))

Comment 6

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

(In reply to David Major [:dmajor] from comment #4)
> On this topic, I happen to be doing some renaming over in bug 1346078. Would
> you like me to hold off on landing that and instead make nsSubstring become
> the canonical name in the binary?

Given that you're about to change it, I think that would be preferable.

(In reply to David Major [:dmajor] from comment #5)
> (FWIW I haven't worried about cpp/h distinctions in years. LTCG/WPO are
> pretty amazing. :-))

Do we have them working anywhere other than Windows?

Comment 7

[(Away)]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #6)
> > (FWIW I haven't worried about cpp/h distinctions in years. LTCG/WPO are
> > pretty amazing. :-))
> 
> Do we have them working anywhere other than Windows?

I'd hope so, but fair point. I'll check, and if we don't, I'll make noise and/or bugs.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb64cd825149
Use nsDependentSubstring for the substring splitter's pieces. r=dbaron

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb64cd825149