Closed
Bug 698256
Opened 13 years ago
Closed 12 years ago
Want Substring(data, length) convenience method
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: neil, Assigned: neil)
Details
Attachments
(1 file)
14.96 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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•13 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•13 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•13 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•12 years ago
|
||
Pushed changeset 141fe205fb73 to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•