Last Comment Bug 743581 - Remove nsCRT::strlen(const PRUnichar* s) from nsCRT.h
: Remove nsCRT::strlen(const PRUnichar* s) from nsCRT.h
Status: VERIFIED FIXED
: perf
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- minor with 1 vote (vote)
: mozilla15
Assigned To: Shriram (irc: Mavericks) Away
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 150073 337730 786687
Blocks: 124536
  Show dependency treegraph
 
Reported: 2012-04-08 12:22 PDT by Shriram (irc: Mavericks) Away
Modified: 2012-08-29 08:23 PDT (History)
5 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch that removes nsCRT::strlen(const PRUnichar*) (30.02 KB, patch)
2012-05-10 10:18 PDT, Shriram (irc: Mavericks) Away
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
Patch that removes nsCRT::strlen(const PRUnichar*) (30.00 KB, patch)
2012-05-10 19:48 PDT, Shriram (irc: Mavericks) Away
bugzillamozillaorg_serge_20140323: review-
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
Patch that removes nsCRT::strlen(const PRUnichar*) (30.74 KB, patch)
2012-05-19 05:51 PDT, Shriram (irc: Mavericks) Away
dougt: review+
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review

Description Shriram (irc: Mavericks) Away 2012-04-08 12:22:40 PDT
As per comment #23,24 32,33 and 35, nsCRT::strlen should be removed.
Comment 1 Shriram (irc: Mavericks) Away 2012-04-08 12:24:45 PDT
Comments mentioned above are from Bug 150073
Comment 2 Serge Gautherie (:sgautherie) 2012-05-04 08:34:40 PDT
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) Away 2012-05-07 11:27:42 PDT
(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) Away 2012-05-10 10:18:41 PDT
Created attachment 622771 [details] [diff] [review]
Patch that removes nsCRT::strlen(const PRUnichar*)
Comment 5 Serge Gautherie (:sgautherie) 2012-05-10 15:22:40 PDT
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) 2012-05-10 15:28:25 PDT
(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) Away 2012-05-10 18:43:37 PDT
(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) Away 2012-05-10 19:48:11 PDT
Created attachment 623018 [details] [diff] [review]
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) Away 2012-05-10 19:56:41 PDT
So, the try submission's @ https://tbpl.mozilla.org/?tree=Try&rev=9e3a2601d7f5
Comment 10 Shriram (irc: Mavericks) Away 2012-05-16 18:17:18 PDT
The latest try submission @ https://tbpl.mozilla.org/?tree=Try&rev=946a73cb71e7
Comment 11 Serge Gautherie (:sgautherie) 2012-05-17 11:38:47 PDT
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) 2012-05-17 11:42:21 PDT
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] 2012-05-17 14:24:36 PDT
(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) 2012-05-17 21:33:47 PDT
(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
jford@mozilla.com – Wed May 9 17:27:32 2012 PDT
which already had ... bug 754001!
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-05-18 06:26:05 PDT
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) Away 2012-05-19 05:51:36 PDT
Created attachment 625400 [details] [diff] [review]
Patch that removes nsCRT::strlen(const PRUnichar*)
Comment 18 Ed Morley [:emorley] 2012-05-31 06:33:36 PDT
https://hg.mozilla.org/mozilla-central/rev/f2796ce2a175
Comment 19 Manoj 2012-05-31 21:52:59 PDT
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) 2012-06-01 01:23:46 PDT
V.Fixed, per MXR search.

*****

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

Yes, please.

Note You need to log in before you can comment on or make changes to this bug.