58221 - don't use strlen to check if a string is of length 0

Status: RESOLVED INACTIVE

(Core :: General, defect, P3)

Description

[(not reading, please use [e-mail] instead)]

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.

Comment 1

[Asa Dotzler [:asa]]

seth, is this bug still useful?  

Comment 2

[Håkan Waara]

r=hwaara

Comment 3

[Denis Antrushin]

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

[Denis Antrushin]

removing myself from cc:

Comment 5

[Aaron Lehmann]

Attachment: strlen removal patch

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 6

[Aaron Lehmann]

Attachment: applies to trunk

rediffed

Comment 7

[Aaron Lehmann]

Attachment: another rediff

Comment 8

[Boris Zbarsky [:bzbarsky]]

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?

Comment 9

[timeless]

Attachment: -u10 should be up to review

i'll look to see if i can find any newly introduced offenders...

Comment 10

[Aaron Lehmann]

Attachment: latest diff

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, and I will try to be much quicker with the next review, now that I have read
the whole patch.

Comment 13

[Aaron Lehmann]

Attachment: update

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

-    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

[timeless]

Comment on attachment 105444 [details] [diff] [review]
update

r=timeless

Comment 16

[Aaron Lehmann]

cc dmose (sorry for the spam)

Comment 17

[mmc]

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

[Boris Zbarsky [:bzbarsky]]

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

[Aaron Lehmann]

Were the portions of the patch applying to security/ applied?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Ah.  Not all of them.  The security/manager ones were, but NSS is restricted.

Over to NSS to land this.

Comment 21

[timeless]

Attachment: nss

Comment 22

[timeless]

Attachment: directory

Comment 23

[timeless]

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

[Wan-Teh Chang]

I will take care of the NSS patch.  Assigned the bug back.

Comment 25

[timeless]

Comment on attachment 117634 [details] [diff] [review]
directory

needs checkin

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 117634 [details] [diff] [review]
directory

directory patch checked in r+sr=dmose on both trunk and client branch

Comment 27

[mmc]

wtc: what's the status of the nss patch?

Comment 28

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 31

[Serge Gautherie (:sgautherie)]

Filter on "Nobody_NScomTLD_20080620"

Comment 32

[Mark Banner (:standard8)]

(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).

Comment 33

[Shriram (irc: Mavericks)]

(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

Comment 34

[Jake Senne]

Attachment: Bug 58221 - Change strlen(foo) == 0 to *foo == '\0\' r?#necko-reviewers

Comment 35

[Phabricator Automation]

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.

Comment 36

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 37

[Sylvestre Ledru [:Sylvestre]]

no activity for a while and probably fixed already