Open
Bug 58221
Opened 24 years ago
Updated 4 months ago
don't use strlen to check if a string is of length 0
Categories
(Core :: General, defect, P3)
Core
General
Tracking
()
NEW
People
(Reporter: sspitzer, Unassigned)
References
Details
(Keywords: perf)
Attachments
(9 obsolete files)
from an email from mark stankus, Trivial improvement to performance: file:nsMsgFolderCache.cpp sspitzer 1.25 lines: (sept 7, 2000 11:15 pm) 387 if (nsCRT::strlen(pathKey) == 0) { 388 return NS_ERROR_FAILURE; 389 } slightly more efficient: 387 if (*pathKey=='\0') { 388 return NS_ERROR_FAILURE; 389 } logging a bug about this so I remember to check the rest of the code for stuff like this. thanks mark.
QA Contact: esther → stephend
Updated•23 years ago
|
Hardware: PC → All
Comment 3•23 years ago
|
||
Currently, in the following files (nsCRT::)strlen(foo) is compared with 0: addrbook/src/nsAbAddressCollecter.cpp addrbook/src/nsAddrDatabase.cpp addrbook/src/nsDirPrefs.cpp base/search/src/nsMsgSearchAdapter.cpp base/search/src/nsMsgSearchTerm.cpp base/src/nsMessengerMigrator.cpp base/src/nsMsgAccountManager.cpp base/src/nsMsgFolderCache.cpp base/util/nsMsgIdentity.cpp base/util/nsMsgIncomingServer.cpp base/util/nsMsgFolder.cpp compose/src/nsMsgCopy.cpp compose/src/nsMsgCompose.cpp compose/src/nsSmtpProtocol.cpp compose/src/nsSmtpService.cpp db/msgdb/src/nsMsgDatabase.cpp db/msgdb/src/nsMsgHdr.cpp imap/src/nsIMAPNamespace.cpp imap/src/nsImapProtocol.cpp imap/src/nsImapService.cpp imap/src/nsImapUrl.cpp imap/src/nsImapMailFolder.cpp imap/src/nsImapIncomingServer.cpp import/outlook/src/nsOutlookMail.cpp local/src/nsLocalMailFolder.cpp local/src/nsMovemailService.cpp mime/cthandlers/vcard/mimevcrd.cpp mime/src/mimei.cpp mime/src/mimetpfl.cpp mime/src/mimetpla.cpp news/src/nsNewsFolder.cpp news/src/nsNntpIncomingServer.cpp Few more files use strlen ineffisiently (e.g. nsCRT::strlen of const string, multiple strlen on same string etc.)
Comment 4•22 years ago
|
||
removing myself from cc:
Comment 5•22 years ago
|
||
This patch removes uses of {PL_,nsCmd::,}strlen all over the tree where its output is compared to 0. The function calls are replaced with comparisons between the 0th character of a string and the NULL character.
Comment 7•22 years ago
|
||
Attachment #100754 -
Attachment is obsolete: true
Comment 8•22 years ago
|
||
OK. Based on a quick skim: 1) Don't do the comparison to '\0'. Just test *foo as a boolean; any platform on which '\0' does not evaluate to false we don't run on anyway. content/xml/document/src/nsXMLDocument.cpp -- that code looks fucked if GetLocalizedUnicharPref ever fails... want to fix that while you're in there? (a "no" is an acceptable answer if you'd really rather not, but then file a bug on me). > + if((nsnull == valueInUTF8) || *valueInUTF8 == '\0') if ( !valueInUTF8 || !*valueInUTF8) -- simplicity all over. ;) > + if( (mPrintFile == nsnull) || *mPrintFile == '\0' ) same > + if( (s != NULL) && *s != '\0') same. > + if (*((const char *) arbitraryHeaderTerm) != '\0') arbitraryHeaderTerm is an nsXPIDLCString. Those have a convenient IsEmpty() method that returns a boolean. Use that, please, instead of this casting stuff (yes, I know the existing code did it....) > + if (!(const char*) username || *((const char*)username) == '\0') { Again, IsEmpty() > + *defaultServerKey != 0) { And again. > + PRBool redirectLogon = ((const char *) redirectorType && *((const char *) redirectorType) != '\0'); Here too. > + *((const char*) password) == '\0') And here. > + if (! (onlineName.get()) || *(onlineName.get()) == '\0' And here > + if ((const char*) onlineName && *((const char *) onlineName) != '\0') And here. ;) > + if (NS_SUCCEEDED(rv) && *oldMsgId != '\0') And here. > + if ((const char *)onlineName == nsnull || *((const char *) onlineName) == '\0') And here. > + if ((const char *) folderName && *folderName != 0) And here. In fact, I'll stop commenting on those now, especially since in many places the patch does not cover enough of the file (diff -u6 next time please?) to see what the variable is declared as. Just make sure all users of nsCAutoString or nsXPIDLCString are using IsEmpty() instead of getting out a char* and playing with it. > + if((szText == NULL) || *szText == '\0') Fix the first part of that to not suck too? > + if (*PREF_FILE_NAME_IN_4x == '\0') { Make sure that macro never expands to anything that would require parens around it? Or just add the parens?
i'll look to see if i can find any newly introduced offenders...
Attachment #102297 -
Attachment is obsolete: true
Comment 10•22 years ago
|
||
I searched the tree for new instances of this construct. This patch by timeless fixes all of them, except some in sample plugins. I modified the patch so it would actually compile.
Comment 11•22 years ago
|
||
Comment on attachment 102406 [details] [diff] [review] latest diff My apologies for the comments being slightly out of patch order... > - if(nsCRT::strlen(detector_name) > 0) { > + if(*detector_name) { > - if(nsCRT::strlen(detector_name) > 0) { > + if(*detector_name) { (2 different places). nsCRT::strlen is null-safe... this would crash, imo. > - if (nsCRT::strlen(aUStr) == 0) { > + if (!*aUStr) { > - if (nsCRT::strlen(aUStr) == 0) { > + if (!*aUStr) { (2 places) Can aUStr be null? (diff -u6 next time, please!) > + if (!*buf || buf[0] == '!') This is looking at the exact same byte on both sides of the ||; how about addressing them consistently? ;) > - if (strlen(lts->cookie) > 0) { > + if (*lts->cookie) { if (ltermSendChar(lts, lts->cookie, strlen(lts->cookie)) != 0) Wanna fix that last arg as well? It should probably be "*lts->cookie != '\0'" (need the comparison so we're passing a valid PRBool value). > value = defaultValue ? nsCRT::strdup(defaultValue) : nsnull; > } >- if (PL_strlen(value) == 0) >+ if (!*value) That's gonna crash if defaultValue is false... PL_strlen is null-safe and returns 0 for null, as it happens. > - if (PL_strlen (server->serverName) == 0) > + if (!*server->serverName) Similar issue here. > - if (PL_strlen(*aPassword) > 0) > + if (**aPassword) And here. > - if (PL_strlen(*aPassword) > 0) { > + > + if (**aPassword) { You know the drill. ;) > - if (PL_strlen(*aUsername) > 0) > + > + if (**aUsername) Again. > - if (PL_strlen(m_prefix) == 0) > + if (!*m_prefix) And here. > - Recycle(valueInUTF8); > + if(!valueInUTF8) > return; > + if(!*valueInUTF8) > + { > + Recycle(valueInUTF8); What's with the whitespace? Please use tabs or spaces consistently with the existing code. > + if (!valueInUTF8) > + {} > + else if (!*valueInUTF8) how about: if (valueInUTF8) { if (!*valueInUTF8) { Recycle(valueInUTF8); } else { // existing code } } (and reindent the existing code, of course). > - if (ns && (PL_strlen(ns->GetPrefix()) == 0) && > + if (ns && !*(ns->GetPrefix()) && Null-check ns->GetPrefix()? (store in a temporary) > - nsCRT::strlen(buffer) == 0 && > + !*buffer && nsCRT::strlen is null-safe... Check whether that matters? > + if(!filename || !*filename|| (strlen(filename) > _MAX_PATH)) Space after *filename? > PRUnichar *result = nsnull; > if (NS_SUCCEEDED(rv = gTextToSubURI->UnEscapeAndConvert(mEncoding.get(), filename.get(), &result)) && (result)) { >- if (nsCRT::strlen(result) > 0) { >+ if (*result) { Could you just switch this code to nsXPIDLString? Or is it passing "result" out to someone? > + if ((*temp == '#') || !*temp) remove the extraneous parens > - if (!tPath || PL_strlen(tPath) == 0) > + if (!tPath || !*tPath) Don't bother with that change; that code will not exist once the tree reopens. ;) > - if (0<strlen(key) && strncmp(key, inKey, strlen(inKey)) == 0) > + if (*key && strncmp(key, inKey, strlen(inKey)) == 0) { > if(key) > DisposePtr(key); This code assumes strlen() and strncmp() are null-safe (and your change assumes key != NULL. Are these valid assumptions?
Attachment #102406 -
Flags: needs-work+
Comment 12•22 years ago
|
||
Oh, and I will try to be much quicker with the next review, now that I have read the whole patch.
Comment 13•22 years ago
|
||
This should address all of your comments. I reviewed the safety of assuming pointers are non-null in all places where your comments suggested it.
Attachment #102406 -
Attachment is obsolete: true
Comment 14•22 years ago
|
||
- if (docURLStrPS && nsCRT::strlen(docURLStrPS) > 0) { + if (docURLStrPS && *docURLStrPS) { What happened to the indentation here? tabs? + if (!*valueInUTF8) Recycle (valueInUTF8); This is weirdly indented too.... + char *prefix = ns->GetPrefix(); + if (ns && (!prefix || !*prefix) && What if "ns" is null? GetPrefix() actually gets inlined and is cheap, so I'd be OK with just calling it twice, though I'm not sure whether *(ns->GetPrefix()) would do the right thing (like compile)... Failing all else, just leave this one be for now. In security/nss/cmd/certutil/certutil.c: + if (*buffer != 0) { drop the "!= 0" part. sr=bzbatsky with those four changes.
Comment 15•22 years ago
|
||
Comment on attachment 105444 [details] [diff] [review] update r=timeless
Attachment #105444 -
Flags: review+
Comment 16•22 years ago
|
||
cc dmose (sorry for the spam)
Comment 17•21 years ago
|
||
What is the status of this bug? Is the patch still working or does it need to be updated? Who is willing to sr= it?
Comment 18•21 years ago
|
||
See http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2002-11-16+21%3A00%3A00&maxdate=2002-11-16+23%3A59%3A59&cvsroot=%2Fcvsroot -- this patch was checked in on Nov 16, 2002. Any reason not to mark this fixed? ;)
Comment 19•21 years ago
|
||
Were the portions of the patch applying to security/ applied?
Comment 20•21 years ago
|
||
Ah. Not all of them. The security/manager ones were, but NSS is restricted. Over to NSS to land this.
Assignee: sspitzer → wtc
Component: Mail Back End → Libraries
Product: MailNews → NSS
QA Contact: stephend → bishakhabanerjee
Version: Trunk → 3.0
Comment 21•21 years ago
|
||
Attachment #102360 -
Attachment is obsolete: true
Attachment #105444 -
Attachment is obsolete: true
Comment 22•21 years ago
|
||
Comment 23•21 years ago
|
||
as for mailnews, there's one file left in my checkin directory: mailnews/imap/src/nsImapUrl.cpp It had merge conflicts or something which probably forced me to omit it. Or perhaps i wanted to land it with some other bug (the relevant function uses PR_Malloc w/ nsCRT::Free and doesn't complain about OOM).
Comment 24•21 years ago
|
||
I will take care of the NSS patch. Assigned the bug back.
Assignee: wtc → sspitzer
Component: Libraries → Mail Back End
Product: NSS → MailNews
QA Contact: bishakhabanerjee → stephend
Version: 3.0 → Trunk
Comment 25•21 years ago
|
||
Comment on attachment 117634 [details] [diff] [review] directory needs checkin
Attachment #117634 -
Flags: review?(dmose)
Comment 26•21 years ago
|
||
Comment on attachment 117634 [details] [diff] [review] directory directory patch checked in r+sr=dmose on both trunk and client branch
Attachment #117634 -
Attachment is obsolete: true
Attachment #117634 -
Flags: review?(dmose) → review+
Comment 27•21 years ago
|
||
wtc: what's the status of the nss patch?
Attachment #117633 -
Flags: review?(wchang0222)
Updated•20 years ago
|
Product: MailNews → Core
Comment 28•17 years ago
|
||
sorry for the spam. making bugzilla reflect reality as I'm not working on these bugs. filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Comment 29•17 years ago
|
||
Comment on attachment 117633 [details] [diff] [review] nss Looking at the current NSS trunk, this patch is completely obsolete. 3 of the 4 hunks no longer exist and the 4th already makes the change the patch is proposing.
Attachment #117633 -
Attachment is obsolete: true
Attachment #117633 -
Flags: review?(wtc)
Comment 30•17 years ago
|
||
With attachment 117633 [details] [diff] [review] no longer relevant, the following appear to still be uses this bug refers to. However, I'm not entirely certain that's the case. http://mxr.mozilla.org/mozilla/source/modules/softupdt/src/nsInstallFile.cpp#161 http://mxr.mozilla.org/mozilla/source/netwerk/protocol/http/src/nsHttpAuthCache.cpp#358 http://mxr.mozilla.org/mozilla/source/netwerk/protocol/http/src/nsHttpAuthCache.cpp#485 http://mxr.mozilla.org/mozilla/source/content/svg/content/src/nsSVGTransformListParser.cpp#126 timeless, do any of these still need changing or can this bug be resolved?
Assignee | ||
Updated•16 years ago
|
Product: Core → MailNews Core
Comment 32•16 years ago
|
||
(In reply to comment #30) > With attachment 117633 [details] [diff] [review] no longer relevant, the following appear to still be > uses this bug refers to. However, I'm not entirely certain that's the case. http://mxr.mozilla.org/mozilla/source/modules/softupdt/src/nsInstallFile.cpp#161 http://mxr.mozilla.org/mozilla/source/content/svg/content/src/nsSVGTransformListParser.cpp#126 These two are still relevant IMHO, which makes this a core bug, not a MailNews one (at a very quick glance, I also couldn't see any specific mailnews cases).
Component: Backend → General
Product: MailNews Core → Core
QA Contact: backend → general
Comment 33•12 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #32) > http://mxr.mozilla.org/mozilla/source/modules/softupdt/src/nsInstallFile. > cpp#161 > http://mxr.mozilla.org/mozilla/source/content/svg/content/src/ > nsSVGTransformListParser.cpp#126 I can't find those in dxr.lanedo.com
Updated•2 years ago
|
Severity: trivial → S4
Comment 34•10 months ago
|
||
Updated•10 months ago
|
Assignee: nobody → grover7677
Status: NEW → ASSIGNED
Updated•10 months ago
|
Attachment #9341410 -
Attachment description: Bug 58221 - Change strlen(foo) == 0 to *foo == '\0\ r?#necko-reviewers → Bug 58221 - Change strlen(foo) == 0 to *foo == '\0\' r?#necko-reviewers
Comment 35•10 months ago
|
||
Comment on attachment 9341410 [details]
Bug 58221 - Change strlen(foo) == 0 to *foo == '\0' r?#necko-reviewers
Revision D182285 was moved to bug 1840882. Setting attachment 9341410 [details] to obsolete.
Attachment #9341410 -
Attachment is obsolete: true
Comment 36•4 months ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: grover7677 → nobody
Status: ASSIGNED → NEW
You need to log in
before you can comment on or make changes to this bug.
Description
•