698256 - Want Substring(data, length) convenience method

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[[e-mail]]

While writing the patch for bug 638031 I bemoaned a lack of a Rebind(data, length) method for nsDependentString. I realised that there wasn't a Substring(data, length) either. There are a number of places in the code base that are forced to use Substring(data, data + length) which is silly because nsDependentString then has to calculate end - start anyway.

Interestingly the external API does provide Substring(data, length).

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

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

Comment on attachment 570538 [details] [diff] [review]
Proposed patch

>diff --git a/xpcom/string/public/nsTDependentSubstring.h b/xpcom/string/public/nsTDependentSubstring.h
>--- a/xpcom/string/public/nsTDependentSubstring.h
>+++ b/xpcom/string/public/nsTDependentSubstring.h
>@@ -54,24 +54,32 @@ class nsTDependentSubstring_CharT : publ
>     public:
> 
>       typedef nsTDependentSubstring_CharT    self_type;
> 
>     public:
> 
>       void Rebind( const substring_type&, PRUint32 startPos, PRUint32 length = size_type(-1) );
> 
>-      void Rebind( const char_type* start, const char_type* end );
>+      void Rebind( const char_type* data, size_type length );
>+
>+      void Rebind( const char_type* start, const char_type* end )
>+        {
>+          Rebind(start, PRUint32(end - start));
>+        }
> 
>       nsTDependentSubstring_CharT( const substring_type& str, PRUint32 startPos, PRUint32 length = size_type(-1) )
>         : substring_type()
>         {
>           Rebind(str, startPos, length);
>         }
> 
>+      nsTDependentSubstring_CharT( const char_type* data, PRUint32 length )
>+        : substring_type(const_cast<char_type*>(data), length, F_NONE) {}
>+

Could you prefer size_type everywhere you've added a PRUint32.

>diff --git a/xpcom/string/src/nsTDependentSubstring.cpp b/xpcom/string/src/nsTDependentSubstring.cpp
>--- a/xpcom/string/src/nsTDependentSubstring.cpp
>+++ b/xpcom/string/src/nsTDependentSubstring.cpp
>@@ -49,19 +49,22 @@
> 
>     mData = const_cast<char_type*>(str.Data()) + startPos;
>     mLength = NS_MIN(length, strLength - startPos);
> 
>     SetDataFlags(F_NONE);
>   }
> 
> void
>-nsTDependentSubstring_CharT::Rebind( const char_type* start, const char_type* end )
>+nsTDependentSubstring_CharT::Rebind( const char_type* data, PRUint32 length )

size_type again, I think

>+    if (length == size_type(-1))
>+      length = char_traits::length(data);

I don't think you should add this -- I don't see any good reason for it, and it's an extra test.  Instead, I'd suggest an NS_ASSERTION that it's not size_type(-1).


r=dbaron with that.

Comment 3

[[e-mail]]

(In reply to David Baron from comment #2)
> (From update of attachment 570538 [details] [diff] [review])
> >+    if (length == size_type(-1))
> >+      length = char_traits::length(data);
> I don't think you should add this -- I don't see any good reason for it, and
> it's an extra test.
It turns out that I accidentally omitted one of the potential users.
nsStringInputStream::SetData directly calls Assign.
nsStringInputStream::AdoptData directly calls Adopt.
So I thought nsStringInputStream::ShareData should directly call Rebind.
Currently it has to compute the length to call the existing version.
It therefore already special-cases -1 so I could leave that code in.
Alternatively, there's only one consumer of ShareData that passes -1.
That's despite already knowing the length of the string to share...
Of course this would mean changing the IDL slightly to prohibit -1.

Comment 4

[[e-mail]]

On the other hand, nsTDependentString::Rebind doesn't accept -1 either.
So I'll leave the check in nsStringInputStream for now.
Is it OK to flip the Rebind call itself though?

Comment 5

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

What does "flip the Rebind call" mean?

Comment 6

[[e-mail]]

(In reply to David Baron from comment #5)
> What does "flip the Rebind call" mean?
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsStringStream.cpp#207
"Flip" from mData.Rebind(data, data + dataLen); to mData.Rebind(data, dataLen);

Comment 7

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

(In reply to [e-mail] from comment #6)
> (In reply to David Baron from comment #5)
> > What does "flip the Rebind call" mean?
> http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsStringStream.cpp#207
> "Flip" from mData.Rebind(data, data + dataLen); to mData.Rebind(data,
> dataLen);

That seems fine.  (Not sure what requires it, though.)

Comment 8

[[e-mail]]

Pushed changeset 141fe205fb73 to mozilla-central.