81762 - Excessive use of strlen

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P3)

Description

[Ulrich Drepper]

The files

  appshell/src/nsCommandLineService.cpp
  appshell/src/nsCommandLineServiceMac.cpp
  appshell/src/nsFileLocations.cpp
  appshell/src/nsUserInfoUnix.cpp

contain unnecessary uses of the function nsCRT::strlen which cannot be
optimized out by the compiler since it doesn't know the function is the
same as strlen.

The patch mostly replaces expressions like

  strlen(foo) > 0

with

  foo[0] != '\0'

or something similar.  Only in the file

  appshell/src/nsCommandLineServiceMac.cpp

there is another optimization.  The handling of the kCommandLinePrefix
and kEnvVarLinePrefix were less than optimal.  The changes remove two
function calls and two extra variables (the pointers, simply define
two arrays).

The patch is tested except for the Macintosh portion.

Comment 1

[Ulrich Drepper]

Attachment: strlen optimizations

Comment 2

[Paul Chen]

nav triage team:

Not a beta stopper, marking nsbeta1- and marking future

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

For whatever it's worth, the patch looks just fine to me (r=[e-mail],
not that I'm an approved reviewer).

Severity -> minor
Nominating for 1.0; it's a trivial & safe patch if it hasn't bit-rotted too badly.
Probably should remove nsbeta1-
I advise dropping Milestone: future.

Comment 4

[Paul Chen]

removing nsbeta1-, marking p3 and mozilla0.9.6

Comment 5

[Paul Chen]

cc-ing Simon to look over the patch and give an sr= if it meets with his approval

Comment 6

[Simon Fraser [no longer active]]

One difference between PR_strlen and strlen is that the former deals gracefully 
with null strings. That patch should be reviewed with that in mind. rs=sfraser

Comment 7

[Paul Chen]

Attachment: updated patch

Comment 8

[Paul Chen]

Just posted a new patch, main difference is that I don't change from PL_strlen
if we don't first check for null.

Comment 9

[Ulrich Drepper]

The new patch isn't correct.  Take this:

-            (void)AddToCommandLine(chars + PL_strlen(kCommandLinePrefix));
+            (void)AddToCommandLine(chars + sizeof(kCommandLinePrefix));

This patch changes the behavior since sizeof() counts the terminating \0 byte in
while strlen of course doesn't.  If you look at my patch you'll see that I added
-1 to compensate for that.

Comment 10

[Paul Chen]

Attachment: ooops, forgot the -1

Comment 11

[Paul Chen]

yet another patch, thanks for catching that one

Comment 12

[Paul Chen]

Attachment: hopefully last patch

Comment 13

[Paul Chen]

Ok, new patch. This one is the same as the first patch submitted by Ulrich, the
difference is that I add checks for null pointer where needed.

Comment 14

[jag (Peter Annema)]

Comment on attachment 55942 [details] [diff] [review]
hopefully last patch

@@ -168,7 +168,7 @@
     rv = GetDomain(getter_Copies(domain));
     if (NS_FAILED(rv)) return rv;
 
-    if ((const char *)username && (const char*)domain && nsCRT::strlen((const
char *)username) && nsCRT::strlen((const char *)domain)) {
+    if ((const char *)username && (const char*)domain && ((const char
*)username)[0] && ((const char *)domain)[0]) {
         emailAddress = (const char *)username;
         emailAddress += "@";
         emailAddress += (const char *)domain;

|domain| and |username| are |nsXPIDLCString|s, so you can just check
|IsEmpty()|.

Fix that and r=jag.

Comment 15

[Paul Chen]

Attachment: updated nsUserInfoUnix.cpp patch with jag's suggestion

Comment 16

[Paul Chen]

Comment on attachment 56014 [details] [diff] [review]
updated nsUserInfoUnix.cpp patch with jag's suggestion

jag said r= if I updated this part

Comment 17

[Simon Fraser [no longer active]]

sr=sfraser

Comment 18

[Paul Chen]

checked into trunk and I fixed the blah.IsEmpty() to !blah.IsEmpty() error in my
last patch

Comment 19

[John Morrison]

vrfy (checked in).