Want Substring(data, length) convenience method

RESOLVED FIXED

Status

()

Core
String
--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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).
(Assignee)

Comment 1

6 years ago
Created attachment 570538 [details] [diff] [review]
Proposed patch
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
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
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?
(Assignee)

Comment 6

6 years ago
(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.)
(Assignee)

Comment 8

6 years ago
Pushed changeset 141fe205fb73 to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.