743581 - Remove nsCRT::strlen(const PRUnichar* s) from nsCRT.h

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Shriram (irc: Mavericks)]

As per comment #23,24 32,33 and 35, nsCRT::strlen should be removed.

Comment 1

[Shriram (irc: Mavericks)]

Comments mentioned above are from Bug 150073

Comment 2

[Serge Gautherie (:sgautherie)]

http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsCRT.h#182
{
183   static PRUint32 strlen(const PRUnichar* s) {
184     // XXXbsmedberg: remove this null-check at some point
185     if (!s) {
186       NS_ERROR("Passing null to nsCRT::strlen");
187       return 0;
188     }
189     return NS_strlen(s);
190   }
}

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCRTGlue.cpp#103
{
104 PRUint32
105 NS_strlen(const PRUnichar *aString)
106 {
107   const PRUnichar *end;
108 
109   for (end = aString; *end; ++end) {
110     // empty loop
111   }
112 
113   return end - aString;
114 }
}

This bug should mostly be as simple as s/nsCRT::strlen/NS_strlen/g...

*****

Though I'm a little confused by (for example)
http://mxr.mozilla.org/comm-central/source/mozilla/extensions/spellcheck/hunspell/src/mozHunspell.cpp#536
{
551   char ** wlst;

560         PRInt32 inLength = nsCRT::strlen(wlst[index]);
}
as I would have expected all remaining calls to have PRUnichar args...

Comment 3

[Shriram (irc: Mavericks)]

(In reply to Serge Gautherie (:sgautherie) from comment #2)
> Though I'm a little confused by (for example)
> http://mxr.mozilla.org/comm-central/source/mozilla/extensions/spellcheck/
> hunspell/src/mozHunspell.cpp#536
> {
> 551   char ** wlst;
> 
> 560         PRInt32 inLength = nsCRT::strlen(wlst[index]);
> }
> as I would have expected all remaining calls to have PRUnichar args...

Not anymore I guess, unless I'm missing something

Comment 4

[Shriram (irc: Mavericks)]

Attachment: Patch that removes nsCRT::strlen(const PRUnichar*)

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 622771 [details] [diff] [review]
Patch that removes nsCRT::strlen(const PRUnichar*)

Review of attachment 622771 [details] [diff] [review]:
-----------------------------------------------------------------

Did this succeed on Try?

::: intl/unicharutil/src/nsSaveAsCharset.cpp
@@ +203,5 @@
>  
>    *outString = NULL;
>  
>    nsresult rv;
> +  PRInt32 inStringLength = NS_strlen(inString);   // original input string length

Keep the comment aligned.

::: layout/base/nsBidi.cpp
@@ +280,5 @@
>      return NS_ERROR_INVALID_ARG;
>    }
>  
>    if(aLength==-1) {
> +    aLength=NS_strlen(aText);

Add missing spaces.

::: layout/printing/nsPrintEngine.cpp
@@ +2416,2 @@
>      if (aDoFront) {
> +      PRUnichar * ptr = &aStr[NS_strlen(aStr)-aLen+3];

Add missing spaces.

::: netwerk/streamconv/converters/mozTXTToHTMLConv.cpp
@@ +1093,5 @@
>        // sendmail/mbox
>        // Placed here for performance increase
>        const PRUnichar * indexString = &line[logLineStart];
>             // here, |logLineStart < lineLength| is always true
> +      PRUint32 minlength = MinInt(6,NS_strlen(indexString));

Add missing spaces.

::: xpcom/ds/nsCRT.h
@@ +177,5 @@
>      }
>      * WARNING - STRTOK WHACKS str THE FIRST TIME IT IS CALLED *
>      * MAKE A COPY OF str IF YOU NEED TO USE IT AFTER strtok() *
>    */
> +  static char* strtok(char* str, const char* delims, char* *newStr);

Right, but leave this line alone wrt this bug.

::: xpcom/ds/nsVariant.cpp
@@ +442,5 @@
>                  if(str)
>                  {
>                      if(nsnull == (*(outp++) = (PRUnichar*)
>                         nsMemory::Clone(str,
> +                        (NS_strlen(str)+1)*sizeof(PRUnichar))))

Add missing spaces.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to Shriram (irc: Mavericks) from comment #3)
> (In reply to Serge Gautherie (:sgautherie) from comment #2)
> > as I would have expected all remaining calls to have PRUnichar args...
> 
> Not anymore I guess, unless I'm missing something

I'm not sure what you meant.
Ftr, could you check and list the calls which have not an (obvious) PRUnichar arg?

Comment 7

[Shriram (irc: Mavericks)]

(In reply to Serge Gautherie (:sgautherie) from comment #6)
> (In reply to Shriram (irc: Mavericks) from comment #3)
> > (In reply to Serge Gautherie (:sgautherie) from comment #2)
> > > as I would have expected all remaining calls to have PRUnichar args...
> > 
> > Not anymore I guess, unless I'm missing something
> 
> I'm not sure what you meant.
> Ftr, could you check and list the calls which have not an (obvious)
> PRUnichar arg?

I meant on 
http://mxr.mozilla.org/comm-central/source/mozilla/extensions/spellcheck/hunspell/src/mozHunspell.cpp#560 
it has strlen(wlst[index]) only

Comment 8

[Shriram (irc: Mavericks)]

Attachment: Patch that removes nsCRT::strlen(const PRUnichar*)

I sent it to try before but hit a Try Syntax Error issue.
I sent one again for this patch with the following:
try: -b do -p all -u all -t none --post-to-bugzilla Bug 743581

Comment 9

[Shriram (irc: Mavericks)]

So, the try submission's @ https://tbpl.mozilla.org/?tree=Try&rev=9e3a2601d7f5

Comment 10

[Shriram (irc: Mavericks)]

The latest try submission @ https://tbpl.mozilla.org/?tree=Try&rev=946a73cb71e7

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 623018 [details] [diff] [review]
Patch that removes nsCRT::strlen(const PRUnichar*)

(In reply to Shriram (irc: Mavericks) from comment #3)
> Not anymore I guess, unless I'm missing something

Hum, right: fixed bug bug 150073.
Must have been some comm-central (delayed) update artefact :-/


(In reply to Shriram (irc: Mavericks) from comment #9)
> https://tbpl.mozilla.org/?tree=Try&rev=9e3a2601d7f5

This run had a failing patch.


(In reply to Shriram (irc: Mavericks) from comment #10)
> https://tbpl.mozilla.org/?tree=Try&rev=946a73cb71e7

This run succeeded.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 623018 [details] [diff] [review]
Patch that removes nsCRT::strlen(const PRUnichar*)

Oh, this _is_ the failing patch.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(In reply to Serge Gautherie (:sgautherie) from comment #11)
> (In reply to Shriram (irc: Mavericks) from comment #10)
> > https://tbpl.mozilla.org/?tree=Try&rev=946a73cb71e7
> 
> This run succeeded.

You sure about that? The Android R3 failures look real (though I don't know if it was an issue with the underlying changeset or the patch itself).

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to Ryan VanderMeulen from comment #13)

> You sure about that? The Android R3 failures look real (though I don't know

Ftr, I usually don't care about Android as it has (or used to have?) so many (random) issues.

> if it was an issue with the underlying changeset or the patch itself).

Try http://hg.mozilla.org/try/rev/946a73cb71e7
was pushed on top of
m-c http://hg.mozilla.org/try/rev/654ac86492e8
which ran at
https://tbpl.mozilla.org/?rev=654ac86492e8
[e-mail] – Wed May 9 17:27:32 2012 PDT
which already had ... bug 754001!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Good, I was hoping it was something like that. Thanks for confirming. Shriram - looks like you're good to go!

Comment 16

[Shriram (irc: Mavericks)]

Attachment: Patch that removes nsCRT::strlen(const PRUnichar*)

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2796ce2a175

:-)

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f2796ce2a175

Comment 19

[Manoj]

While looking at the diff, I noticed a sub-optimal use of NS_Strlen in nsFilePicker: 
https://bugzilla.mozilla.org/attachment.cgi?id=625400&action=diff#a/widget/windows/nsFilePicker.cpp_sec1

NS_Strlen is called twice on current within 2 lines when the code can very easily be refactored so that the length of current is captured in an automatic variable.

Should this be filed as a separate bug?

Comment 20

[Serge Gautherie (:sgautherie)]

V.Fixed, per MXR search.

*****

(In reply to Manoj from comment #19)
> Should this be filed as a separate bug?

Yes, please.