Closed Bug 115289 Opened 23 years ago Closed 23 years ago

[FIX]Convert Compare() to Equals() where possible

Categories

(Core :: XPCOM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla0.9.9

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(2 files, 2 obsolete files)

Compare(str1, str2, comparator) == 0

should be

str1.Equals(str2, comparator)

now that we can do that.
Taking bug.
Assignee: jaggernaut → bzbarsky
Priority: -- → P2
Target Milestone: --- → mozilla0.9.9
Attached patch Patch v1 (obsolete) — Splinter Review
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 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("&gt;"), nsCaseInsensitiveStringComparator()) == 0)
>+		if (Substring(temp, 0, 4).Equals(NS_LITERAL_STRING("&gt;"), nsCaseInsensitiveStringComparator()))
> 		{
> 			text.Cut(offset, 4);
> 			text.Insert(PRUnichar('>'), offset);
> 		}
>-		if (Compare(Substring(temp, 0, 5), NS_LITERAL_STRING("&amp;"), nsCaseInsensitiveStringComparator()) == 0)
>+		if (Substring(temp, 0, 5).Equals(NS_LITERAL_STRING("&amp;"), nsCaseInsensitiveStringComparator()))
> 		{
> 			text.Cut(offset, 5);
> 			text.Insert(PRUnichar('&'), offset);
> 		}
>-		if (Compare(Substring(temp, 0, 6), NS_LITERAL_STRING("&quot;"), nsCaseInsensitiveStringComparator()) == 0)
>+		if (Substring(temp, 0, 6).Equals(NS_LITERAL_STRING("&quot;"), 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 "&lt;&gt;", 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("&lt;"), nsCaseInsensitiveStringComparator()) == 0)
>+		if (Substring(temp, 0, 4).Equals(NS_LITERAL_STRING("&lt;"), nsCaseInsensitiveStringComparator()))
> 		{
> 			text.Cut(offset, 4);
> 			text.Insert(PRUnichar('<'), offset);
> 		}
>-		if (Compare(Substring(temp, 0, 4), NS_LITERAL_STRING("&gt;"), nsCaseInsensitiveStringComparator()) == 0)
>+		if (Substring(temp, 0, 4).Equals(NS_LITERAL_STRING("&gt;"), nsCaseInsensitiveStringComparator()))
> 		{
> 			text.Cut(offset, 4);
> 			text.Insert(PRUnichar('>'), offset);
> 		}
>-		if (Compare(Substring(temp, 0, 5), NS_LITERAL_STRING("&amp;"), nsCaseInsensitiveStringComparator()) == 0)
>+		if (Substring(temp, 0, 5).Equals(NS_LITERAL_STRING("&amp;"), nsCaseInsensitiveStringComparator()))
> 		{
> 			text.Cut(offset, 5);
> 			text.Insert(PRUnichar('&'), offset);
> 		}
>-		if (Compare(Substring(temp, 0, 6), NS_LITERAL_STRING("&quot;"), nsCaseInsensitiveStringComparator()) == 0)
>+		if (Substring(temp, 0, 6).Equals(NS_LITERAL_STRING("&quot;"), nsCaseInsensitiveStringComparator()))
> 		{
> 			text.Cut(offset, 6);
> 			text.Insert(PRUnichar('\"'), offset);

Same comments apply.
Attached patch Patch v1.1Splinter Review
Fix the parts jag just pointed out.
Attachment #67034 - Attachment is obsolete: true
Comment on attachment 67060 [details] [diff] [review]
Patch v1.1

sr=jag
Attachment #67060 - Flags: superreview+
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+
Checked into trunk.  Fear the conflicts.  :)
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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 → ---
Sorry...My prev comment should be....there's no "Equals()" for CString in
MFC..change it back to "Compare()".
Fix for mfcembed checked in.  Should have noticed that was a different string
class...
Status: REOPENED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
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.
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 on attachment 67958 [details] [diff] [review]
patch to fix assertions.

sr=sfraser
Attachment #67958 - Flags: superreview+
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 on attachment 67958 [details] [diff] [review]
patch to fix assertions.

r=bnesse.
Attachment #67958 - Flags: review+
Comment on attachment 67965 [details] [diff] [review]
better patch

r=bzbarsky
Attachment #67965 - Flags: review+
Comment on attachment 67965 [details] [diff] [review]
better patch

sr=sfraser
Attachment #67965 - Flags: superreview+
ok, the last patch is in, so the assertions are gone.
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: