Closed Bug 698256 Opened 8 years ago Closed 8 years ago

Want Substring(data, length) convenience method

Categories

(Core :: String, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

Details

Attachments

(1 file)

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).
Attached patch Proposed patchSplinter Review
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #570538 - Flags: review?(dbaron)
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.
Attachment #570538 - Flags: review?(dbaron) → review+
Component: XPCOM → String
QA Contact: xpcom → string
(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.
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?
What does "flip the Rebind call" mean?
(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);
(In reply to neil@parkwaycc.co.uk 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.)
Pushed changeset 141fe205fb73 to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.