Last Comment Bug 698256 - Want Substring(data, length) convenience method
: Want Substring(data, length) convenience method
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: String (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-30 03:20 PDT by neil@parkwaycc.co.uk
Modified: 2011-12-16 16:18 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (14.96 KB, patch)
2011-10-30 03:35 PDT, neil@parkwaycc.co.uk
dbaron: review+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2011-10-30 03:20:35 PDT
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 neil@parkwaycc.co.uk 2011-10-30 03:35:34 PDT
Created attachment 570538 [details] [diff] [review]
Proposed patch
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-06 17:09:43 PST
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 neil@parkwaycc.co.uk 2011-12-07 01:39:43 PST
(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 neil@parkwaycc.co.uk 2011-12-07 03:39:48 PST
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-7 (review requests must explain patch) 2011-12-07 08:25:50 PST
What does "flip the Rebind call" mean?
Comment 6 neil@parkwaycc.co.uk 2011-12-07 08:30:43 PST
(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-7 (review requests must explain patch) 2011-12-08 13:47:35 PST
(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.)
Comment 8 neil@parkwaycc.co.uk 2011-12-16 16:18:10 PST
Pushed changeset 141fe205fb73 to mozilla-central.

Note You need to log in before you can comment on or make changes to this bug.