Closed
Bug 115289
Opened 23 years ago
Closed 23 years ago
[FIX]Convert Compare() to Equals() where possible
Categories
(Core :: XPCOM, defect, P1)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla0.9.9
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
Attachments
(2 files, 2 obsolete files)
85.82 KB,
patch
|
alecf
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
1003 bytes,
patch
|
bzbarsky
:
review+
sfraser_bugs
:
superreview+
|
Details | Diff | Splinter Review |
Compare(str1, str2, comparator) == 0 should be str1.Equals(str2, comparator) now that we can do that.
Assignee | ||
Comment 1•23 years ago
|
||
Taking bug.
Assignee: jaggernaut → bzbarsky
Priority: -- → P2
Target Milestone: --- → mozilla0.9.9
Assignee | ||
Comment 2•23 years ago
|
||
Assignee | ||
Comment 3•23 years ago
|
||
jag? alecf? Can I bother you for more string mass-change review? :) The patch changes all instances except where Compare actually needs to be used (like people want to sort) and nsFormSubmitter.cpp which is in the middle of a full rewrite (but I asked jkeiser to make similar changes to his rewritten version and he has).
Status: NEW → ASSIGNED
Priority: P2 → P1
Summary: Convert Compare() to Equals() where possible → [FIX]Convert Compare() to Equals() where possible
Comment 4•23 years ago
|
||
Comment on attachment 67034 [details] [diff] [review] Patch v1 >Index: xpfe/components/bookmarks/src/nsBookmarksService.cpp >=================================================================== >RCS file: /cvsroot/mozilla/xpfe/components/bookmarks/src/nsBookmarksService.cpp,v >retrieving revision 1.211 >diff -u -r1.211 nsBookmarksService.cpp >--- xpfe/components/bookmarks/src/nsBookmarksService.cpp 2001/12/17 07:05:59 1.211 >+++ xpfe/components/bookmarks/src/nsBookmarksService.cpp 2002/01/30 03:17:52 >@@ -933,25 +933,24 @@ > { > // XXX get max of 6 chars; change the value below if > // we ever start looking for longer HTML-escaped values >- nsAutoString temp; >- text.Mid(temp, offset, 6); >+ const nsDependentSingleFragmentSubstring& temp = Substring(text, offset, 6); > >- if (Compare(Substring(temp, 0, 4), NS_LITERAL_STRING("<"), nsCaseInsensitiveStringComparator()) == 0) >+ if (Substring(temp, 0, 4).Equals(NS_LITERAL_STRING("<"), nsCaseInsensitiveStringComparator())) > { > text.Cut(offset, 4); > text.Insert(PRUnichar('<'), offset); > } >- if (Compare(Substring(temp, 0, 4), NS_LITERAL_STRING(">"), nsCaseInsensitiveStringComparator()) == 0) >+ if (Substring(temp, 0, 4).Equals(NS_LITERAL_STRING(">"), nsCaseInsensitiveStringComparator())) > { > text.Cut(offset, 4); > text.Insert(PRUnichar('>'), offset); > } >- if (Compare(Substring(temp, 0, 5), NS_LITERAL_STRING("&"), nsCaseInsensitiveStringComparator()) == 0) >+ if (Substring(temp, 0, 5).Equals(NS_LITERAL_STRING("&"), nsCaseInsensitiveStringComparator())) > { > text.Cut(offset, 5); > text.Insert(PRUnichar('&'), offset); > } >- if (Compare(Substring(temp, 0, 6), NS_LITERAL_STRING("""), nsCaseInsensitiveStringComparator()) == 0) >+ if (Substring(temp, 0, 6).Equals(NS_LITERAL_STRING("""), nsCaseInsensitiveStringComparator())) > { > text.Cut(offset, 6); > text.Insert(PRUnichar('\"'), offset); You'll need to change the |if|s to |else if|s. Right now the second |Compare| will match if |text| at index |offset| had "<>", where in the original code it wouldn't. You can also replace these Substring(temp, 0, n) with Substring(text, offset, n), and get rid of |temp|. >Index: xpfe/components/related/src/nsRelatedLinksHandler.cpp >=================================================================== >RCS file: /cvsroot/mozilla/xpfe/components/related/src/nsRelatedLinksHandler.cpp,v >retrieving revision 1.55 >diff -u -r1.55 nsRelatedLinksHandler.cpp >--- xpfe/components/related/src/nsRelatedLinksHandler.cpp 2001/12/23 23:23:35 1.55 >+++ xpfe/components/related/src/nsRelatedLinksHandler.cpp 2002/01/30 03:17:54 >@@ -558,25 +558,24 @@ > { > // XXX get max of 6 chars; change the value below if > // we ever start looking for longer HTML-escaped values >- nsAutoString temp; >- text.Mid(temp, offset, 6); >+ const nsDependentSingleFragmentSubstring& temp = Substring(text, offset, 6); > >- if (Compare(Substring(temp, 0, 4), NS_LITERAL_STRING("<"), nsCaseInsensitiveStringComparator()) == 0) >+ if (Substring(temp, 0, 4).Equals(NS_LITERAL_STRING("<"), nsCaseInsensitiveStringComparator())) > { > text.Cut(offset, 4); > text.Insert(PRUnichar('<'), offset); > } >- if (Compare(Substring(temp, 0, 4), NS_LITERAL_STRING(">"), nsCaseInsensitiveStringComparator()) == 0) >+ if (Substring(temp, 0, 4).Equals(NS_LITERAL_STRING(">"), nsCaseInsensitiveStringComparator())) > { > text.Cut(offset, 4); > text.Insert(PRUnichar('>'), offset); > } >- if (Compare(Substring(temp, 0, 5), NS_LITERAL_STRING("&"), nsCaseInsensitiveStringComparator()) == 0) >+ if (Substring(temp, 0, 5).Equals(NS_LITERAL_STRING("&"), nsCaseInsensitiveStringComparator())) > { > text.Cut(offset, 5); > text.Insert(PRUnichar('&'), offset); > } >- if (Compare(Substring(temp, 0, 6), NS_LITERAL_STRING("""), nsCaseInsensitiveStringComparator()) == 0) >+ if (Substring(temp, 0, 6).Equals(NS_LITERAL_STRING("""), nsCaseInsensitiveStringComparator())) > { > text.Cut(offset, 6); > text.Insert(PRUnichar('\"'), offset); Same comments apply.
Assignee | ||
Comment 5•23 years ago
|
||
Fix the parts jag just pointed out.
Attachment #67034 -
Attachment is obsolete: true
Comment 6•23 years ago
|
||
Comment on attachment 67060 [details] [diff] [review] Patch v1.1 sr=jag
Attachment #67060 -
Flags: superreview+
Comment 7•23 years ago
|
||
Comment on attachment 67060 [details] [diff] [review] Patch v1.1 oh man this is going to stomp on my patch :) land this soon or you'll have to resolve CVS conflicts :)
Attachment #67060 -
Flags: review+
Assignee | ||
Comment 8•23 years ago
|
||
Checked into trunk. Fear the conflicts. :)
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 9•23 years ago
|
||
Hi Boris, Your checkin to fix this bug has busted MfcEmbed since there's no Compare() in the MFC CString class. Could you please change it back to Equals() for the MfcEmbed change as shown below? Reopening. Thanks Chak Index: embedding/tests/mfcembed/PrintSetupDialog.cpp =================================================================== <snip> int CPrintSetupDialog::GetPaperSizeIndex(const CString& aStr) { for (int i=0;i<gNumPaperSizes;i++) { - if (!aStr.Compare(gPaperSize[i].mDesc)) { + if (aStr.Equals(gPaperSize[i].mDesc)) { return i; } }
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 10•23 years ago
|
||
Sorry...My prev comment should be....there's no "Equals()" for CString in MFC..change it back to "Compare()".
Assignee | ||
Comment 11•23 years ago
|
||
Fix for mfcembed checked in. Should have noticed that was a different string class...
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 12•23 years ago
|
||
This checkin appears to be causing nsXBLPrototypeHandler.cpp to throw a very large number of "nsDependentString must wrap a nun-NULL buffer" assertions (on the Mac OS 9 build) due to changing button and clickcount from nsAutoString to nsDependentString objects.
Comment 13•23 years ago
|
||
this is causing some asserts for me, because aButton or aClickCount is null. NTDLL! 77f9f9df() nsDebug::Assertion(const char * 0x1011416c `string', const char * 0x101141a4 `string', const char * 0x101141ac `string', int 54) line 291 + 13 bytes nsDependentString::Rebind(const unsigned short * 0x00000000) line 54 + 28 bytes nsDependentString::nsDependentString(const unsigned short * 0x00000000) line 100 + 47 bytes nsXBLPrototypeHandler::ConstructPrototype(nsIContent * 0x00000000, const unsigned short * 0x017e9c30, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * ...) line 866 nsXBLPrototypeHandler::nsXBLPrototypeHandler(const unsigned short * 0x017e9c30, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000) line 111 NS_NewXBLPrototypeHandler(const unsigned short * 0x017e9c30, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 0x00000000, nsIXBLPrototypeHandler * * ...) line 990 + nsXBLContentSink::ConstructHandler(const unsigned short * * 0x03acfef8) line 464 + 65 bytes nsXBLContentSink::OnOpenContainer(const unsigned short * * 0x03acfef0, unsigned int 1, int 7, nsIAtom * 0x004f02e0) line 326 nsXMLContentSink::HandleStartElement(nsXMLContentSink * const 0x03ac17f8, const unsigned short * 0x03a8c09e, const unsigned short * * 0x03acfef0, unsigned int 1, unsigned int 4294967295, unsigned int 206) line 1214 + 36 bytes nsXBLContentSink::HandleStartElement(nsXBLContentSink * const 0x03ac17f8, const unsigned short * 0x03a8c09e, const unsigned short * * 0x03acfef0, unsigned int 1, unsigned int 4294967295, unsigned int 206) line 195 + 29 bytes nsExpatDriver::HandleStartElement(const unsigned short * 0x03a8c09e, const unsigned short * * 0x03acfef0) line 270 Driver_HandleStartElement(void * 0x03ace320, const char * 0x03a8c09e, const char * * 0x03acfef0) line 68 + 16 bytes doContent(void * 0x03ace0d0, int 0, const encoding * 0x01d29558 little2_encoding, const char * 0x01818790, const char * 0x018192ce, const char * * 0x03ace0dc) line 1405 + 30 bytes contentProcessor(void * 0x03ace0d0, const char * 0x018172a8, const char * 0x018192ce, const char * * 0x03ace0dc) line 1100 + 30 bytes XML_ParseBuffer(void * 0x03ace0d0, int 8192, int 0) line 964 + 64 bytes XML_Parse(void * 0x03ace0d0, const char * 0x0180e140, int 8192, int 0) line 953 + 17 bytes nsExpatDriver::ParseBuffer(const char * 0x0180e140, unsigned int 8192, int 0) line 626 + 24 bytes nsExpatDriver::ConsumeToken(nsExpatDriver * const 0x03ace324, nsScanner & {...}, int & 0) line 737 + 30 bytes nsParser::Tokenize(int 0) line 2588 + 26 bytes nsParser::ResumeParse(int 1, int 0, int 1) line 1846 + 12 bytes nsParser::OnDataAvailable(nsParser * const 0x03ac1194, nsIRequest * 0x03ac1940, nsISupports * 0x00000000, nsIInputStream * 0x03ac24c0, unsigned int 0, unsigned int 16384) line 2470 + 21 bytes nsXBLStreamListener::OnDataAvailable(nsXBLStreamListener * const 0x03ac2fa0, nsIRequest * 0x03ac1940, nsISupports * 0x00000000, nsIInputStream * 0x03ac24c0, unsigned int 0, unsigned int 16384) line 287 + 46 bytes nsJARChannel::OnDataAvailable(nsJARChannel * const 0x03ac1944, nsIRequest * 0x03ac2884, nsISupports * 0x00000000, nsIInputStream * 0x03ac24c0, unsigned int 0, unsigned int 16384) line 638 nsOnDataAvailableEvent::HandleEvent() line 193 + 70 bytes nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x03ac3f34) line 116 PL_HandleEvent(PLEvent * 0x03ac3f34) line 590 + 10 bytes PL_ProcessPendingEvents(PLEventQueue * 0x00491c30) line 520 + 9 bytes _md_EventReceiverProc(HWND__ * 0x00050356, unsigned int 49392, unsigned int 0, long 4791344) line 1071 + 9 bytes USER32! 77e13eb0() USER32! 77e1401a() USER32! 77e192da() nsAppShellService::Run(nsAppShellService * const 0x004b4d00) line 308 main1(int 1, char * * 0x004447f0, nsISupports * 0x00000000) line 1285 + 32 bytes main(int 1, char * * 0x004447f0) line 1625 + 37 bytes mainCRTStartup() line 338 + 17 bytes KERNEL32! 77e87903()
Comment 14•23 years ago
|
||
Comment 15•23 years ago
|
||
Comment on attachment 67958 [details] [diff] [review] patch to fix assertions. sr=sfraser
Attachment #67958 -
Flags: superreview+
Assignee | ||
Comment 16•23 years ago
|
||
Comment on attachment 67958 [details] [diff] [review] patch to fix assertions. Wouldn't something like: if (aButton && *aButton) { mDetail = *aButton - '0'; } and similarly for clickcount work better? There is no reason to create nsAStrings here...
Comment 17•23 years ago
|
||
Comment on attachment 67958 [details] [diff] [review] patch to fix assertions. r=bnesse.
Attachment #67958 -
Flags: review+
Comment 18•23 years ago
|
||
Attachment #67958 -
Attachment is obsolete: true
Assignee | ||
Comment 19•23 years ago
|
||
Comment on attachment 67965 [details] [diff] [review] better patch r=bzbarsky
Attachment #67965 -
Flags: review+
Comment 20•23 years ago
|
||
Comment on attachment 67965 [details] [diff] [review] better patch sr=sfraser
Attachment #67965 -
Flags: superreview+
Comment 21•23 years ago
|
||
ok, the last patch is in, so the assertions are gone.
Updated•3 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•