nsCRT::strcmp(const PRUnichar* s) will be replaced with NS_strcmp @ http://dxr.lanedo.com/mozilla-central/xpcom/glue/nsCRTGlue.h.html#l50

Why is NS_strcmp faster than the compiler's strcmp?

(In reply to Marco Castelluccio [:marco] from comment #1) > Why is NS_strcmp faster than the compiler's strcmp? Less # of comparisons i.e, double check http://dxr.lanedo.com/mozilla-central/xpcom/ds/nsCRT.cpp.html#l93 especially when strings match will help. For reference, NS_strcmp: http://dxr.lanedo.com/mozilla-central/xpcom/glue/nsCRTGlue.cpp.html#l81 Also, comment @ https://bugzilla.mozilla.org/show_bug.cgi?id=786687#c2

(In reply to Shriram (irc: Mavericks) from comment #2) > Less # of comparisons i.e, double check Oh, ok. I didn't see you wanted to remove only the PRUnichar function. Maybe you can remove the null check in http://dxr.lanedo.com/mozilla-central/nsprpub/lib/libc/src/strcmp.c.html#l10 too.

(In reply to Marco Castelluccio [:marco] from comment #3) > Maybe you can remove the null check in > http://dxr.lanedo.com/mozilla-central/nsprpub/lib/libc/src/strcmp.c.html#l10 > too. I agree. I was thinking about that. May be another bug needs to be filed for it. So, PL_strcmp can just be { return (PRIntn)strcmp(a,b); } I looked at removing the const char* function too. It uses PL_strcmp http://dxr.lanedo.com/mozilla-central/xpcom/ds/nsCRT.h.html#l76 Instead of a function call to PL_strcmp, can we replace it with strcmp(s1, s2) directly ? May be that could help

Comment on attachment 669113 [details] [diff] [review] Patch that removes function nsCRT::strcmp(const PRUnichar *) and replaces it with NS_strcmp As with the other bug, NS_strcmp is not null-safe. Did you check the callsites to make sure null is never possible?

This patch removes some changes that belong to a patch for another bug

Checked the calls sites for null

Comment on attachment 691698 [details] [diff] [review] Patch that removes function nsCRT::strcmp(const PRUnichar *) and replaces it with NS_strcmp >--- a/intl/locale/src/nsLocale.cpp > int > nsLocale::Hash_CompareNSString(const void* s1, const void* s2) > { >- return !nsCRT::strcmp((const PRUnichar *) s1, (const PRUnichar *) s2); >+ if (s1 && s2) { >+ return !NS_strcmp((const PRUnichar *) s1, (const PRUnichar *) s2); >+ } >+ else { >+ if (s1) //s2 must have been null >+ return -1; >+ if (s2) //s1 must have been null >+ return 1; >+ return 0; >+ } > } The coding style says "no else after return", so this should be: if (s1 && s2 ) { return !NS_strcmp... } if (s1) { return -1; } if (s2) { return 1; } return 0; And brace all ifs. >--- a/rdf/base/src/nsRDFService.cpp >+++ b/rdf/base/src/nsRDFService.cpp >@@ -168,17 +168,17 @@ struct LiteralHashEntry : public PLDHash > > static bool > MatchEntry(PLDHashTable *table, const PLDHashEntryHdr *hdr, > const void *key) > { > const LiteralHashEntry *entry = > static_cast<const LiteralHashEntry *>(hdr); > >- return 0 == nsCRT::strcmp(static_cast<const PRUnichar *>(key), >+ return key && hdr && 0 == NS_strcmp(static_cast<const PRUnichar *>(key), > entry->mKey); key can never be null here, so you don't need these nullchecks >--- a/security/manager/ssl/src/nsCertOverrideService.cpp >- if (!nsCRT::strcmp(aData, NS_LITERAL_STRING("shutdown-cleanse").get())) { >+ if (aData && !NS_strcmp(aData, NS_LITERAL_STRING("shutdown-cleanse").get())) { If you're interested, there's bug X on removing shutdown-cleanse entirely (it's dead code). r=me with these nits picked. Thanks much!

Addresses nits in last comment

Updated to tip

The checkin? patch flag doesn't do much; you really need the checkin-needed keyword.

Backed out because it broke the build: https://hg.mozilla.org/integration/mozilla-inbound/rev/652b044f22d6

(See https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=66a7359f9be2 for build logs.)

(In reply to :Ehsan Akhgari from comment #16) > (See https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=66a7359f9be2 for > build logs.) Taking a look and will send it try to fix this.