Last Comment Bug 150073 - nsCRT::strlen(const char *) should go away
: nsCRT::strlen(const char *) should go away
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- trivial with 1 vote (vote)
: mozilla15
Assigned To: Shriram (irc: Mavericks) Away
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on: 715938
Blocks: 124536 743581
  Show dependency treegraph
 
Reported: 2002-06-07 16:27 PDT by Dan Mosedale (:dmose)
Modified: 2012-05-04 08:09 PDT (History)
14 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:: for nsCRT::strlen (33.37 KB, patch)
2012-02-18 06:04 PST, Shriram (irc: Mavericks) Away
no flags Details | Diff | Splinter Review
Removes nsCRT:: in nsCRT::strlen (35.32 KB, patch)
2012-03-03 02:42 PST, Shriram (irc: Mavericks) Away
no flags Details | Diff | Splinter Review
Removes nsCRT:: in nsCRT::strlen (36.24 KB, patch)
2012-03-03 18:56 PST, Shriram (irc: Mavericks) Away
no flags Details | Diff | Splinter Review
Removes nsCRT:: in nsCRT::strlen (36.25 KB, patch)
2012-03-03 19:03 PST, Shriram (irc: Mavericks) Away
bugzillamozillaorg_serge_20140323: feedback-
Details | Diff | Splinter Review
Removes nsCRT:: in nsCRT::strlen, changes to nsCRT.h and nsCRT.cpp, and addresses formatting issues (34.81 KB, patch)
2012-03-05 05:47 PST, Shriram (irc: Mavericks) Away
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
Merges a comment in addition to changes from previous patch (34.87 KB, patch)
2012-03-05 07:47 PST, Shriram (irc: Mavericks) Away
doug.turner: review+
Details | Diff | Splinter Review
Patch addresses comment #21 and removes nsCRT::strlen(const char*) occurrences [Same as next attachment] (34.90 KB, patch)
2012-04-02 20:06 PDT, Shriram (irc: Mavericks) Away
no flags Details | Diff | Splinter Review
Patch addresses comment #21 and removes nsCRT::strlen(const char*) occurrences [Backed out: comment 29] (34.90 KB, patch)
2012-04-02 20:09 PDT, Shriram (irc: Mavericks) Away
no flags Details | Diff | Splinter Review
Patch that removes nsCRT::strlen(const char *) from nsCRT.h and removes null check from nsCRT::strlen(const PRUnichar*) [Has obsolete f= and r=] (36.40 KB, patch)
2012-04-07 06:55 PDT, Shriram (irc: Mavericks) Away
no flags Details | Diff | Splinter Review
Patch that removes nsCRT::strlen(const char *) from nsCRT.h and removes null check from nsCRT::strlen(const PRUnichar*) (36.38 KB, patch)
2012-04-07 06:58 PDT, Shriram (irc: Mavericks) Away
bugzillamozillaorg_serge_20140323: feedback-
Details | Diff | Splinter Review
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h (35.82 KB, patch)
2012-04-07 18:13 PDT, Shriram (irc: Mavericks) Away
bugzillamozillaorg_serge_20140323: feedback-
Details | Diff | Splinter Review
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h (36.04 KB, patch)
2012-04-08 09:01 PDT, Shriram (irc: Mavericks) Away
no flags Details | Diff | Splinter Review
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h (14.09 KB, patch)
2012-04-10 08:16 PDT, Shriram (irc: Mavericks) Away
bugzillamozillaorg_serge_20140323: review-
Details | Diff | Splinter Review
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h (13.43 KB, patch)
2012-04-27 06:24 PDT, Shriram (irc: Mavericks) Away
doug.turner: review-
Details | Diff | Splinter Review
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h (12.21 KB, patch)
2012-04-28 01:23 PDT, Shriram (irc: Mavericks) Away
doug.turner: review-
Details | Diff | Splinter Review
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h (8.19 KB, patch)
2012-05-01 13:07 PDT, Shriram (irc: Mavericks) Away
doug.turner: review+
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review

Description Dan Mosedale (:dmose) 2002-06-07 16:27:13 PDT
It's already marked as deprecated in the nsCRT.h comments; we should get rid of
it so that new code can't use it.
Comment 1 Thomas Holaday 2002-12-05 11:09:27 PST
How about a patched version where the deprecated method is private, and all the
current users are declared friends?  That will accomplish two things: 

1) Prevent new code from using it
2) Provide a comprehensive list of current users

I volunteer :-)
Comment 2 Dan Mosedale (:dmose) 2002-12-06 13:57:41 PST
Thomas: why add an extra intermediate step here -- ie why not just get rid of it
entirely?
Comment 3 Thomas Holaday 2002-12-06 14:15:36 PST
The only reason to do it with the intermediate step is to allow the header file
to be changed on one day and the potentially large number of files which
#include the header to be changed on another day.  This minimizes the chance for
collision with a developer making other changes to an #including file.

I am new here and I bring my baggage with me.  Am I overcautious?
Comment 4 Dan Mosedale (:dmose) 2002-12-06 14:24:18 PST
CVS generally handles merges pretty well, I don't think that sort of problem is
likely to be an issue.
Comment 5 Thomas Holaday 2002-12-06 15:06:35 PST
My concern is more with the need of a patch reviewer to consider (at one go)
changes to a large number of files.  I am confidant that CVS would be untroubled.

This being said, I will make the change on a fresh pull and get a count.
Comment 6 Cathleen 2003-01-30 17:58:03 PST
okay, looks like nsCRT::strlen(const char*) are creeping back in mozilla code
again....  ( I should've removed the acutal function, when I had cleaned out all
references a while back! )

Comment 7 Cees T. 2007-01-15 02:32:06 PST
Related: bug 276845.
Comment 8 Serge Gautherie (:sgautherie) 2012-01-06 09:58:27 PST
"Found 52 matching lines in 27 files"
Comment 9 Shriram (irc: Mavericks) Away 2012-02-18 06:04:54 PST
Created attachment 598526 [details] [diff] [review]
Patch that removes nsCRT:: for nsCRT::strlen
Comment 10 Serge Gautherie (:sgautherie) 2012-02-26 23:25:38 PST
Comment on attachment 598526 [details] [diff] [review]
Patch that removes nsCRT:: for nsCRT::strlen

If my count is correct, this patch misses 3 files: /netwerk/protocol/about/nsIAboutModule.idl and 2 more.
Were they missed or is there a reason?
Comment 11 Shriram (irc: Mavericks) Away 2012-02-27 15:47:49 PST
(In reply to Serge Gautherie (:sgautherie) from comment #10)
> If my count is correct, this patch misses 3 files:
> /netwerk/protocol/about/nsIAboutModule.idl and 2 more.
> Were they missed or is there a reason?

I ignored the files where the variable's in the comments.
I will update the patch by including those files too.
Comment 12 Shriram (irc: Mavericks) Away 2012-03-03 02:42:17 PST
Created attachment 602587 [details] [diff] [review]
Removes nsCRT:: in nsCRT::strlen
Comment 13 Serge Gautherie (:sgautherie) 2012-03-03 06:34:49 PST
Comment on attachment 602587 [details] [diff] [review]
Removes nsCRT:: in nsCRT::strlen

If my count is correct, this patch still misses 1 file.
Is there a reason?
Comment 14 Shriram (irc: Mavericks) Away 2012-03-03 18:56:04 PST
Created attachment 602684 [details] [diff] [review]
Removes nsCRT:: in nsCRT::strlen

Includes all matching files now.
Comment 15 Shriram (irc: Mavericks) Away 2012-03-03 19:03:48 PST
Created attachment 602685 [details] [diff] [review]
Removes nsCRT:: in nsCRT::strlen

Has an accurate summary of changes message in the patch.
Comment 16 Serge Gautherie (:sgautherie) 2012-03-04 05:42:49 PST
Comment on attachment 602685 [details] [diff] [review]
Removes nsCRT:: in nsCRT::strlen

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

You should not touch the 2 nsCRT.* files (in this patch).

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

Nit: the comment should remain aligned.

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

Nit: while here, add a space around '='.

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

Nit: while here, add a space around '-' and '+'.

::: 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,strlen(indexString));

Nit: while here, add a space after ','.

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

Nit: while here, add a space around '+' and '*'.
Comment 17 Shriram (irc: Mavericks) Away 2012-03-04 06:30:57 PST
Owh! yes, i shouldn't have touched nsCRT.h due to comment above it.
Comment 18 Shriram (irc: Mavericks) Away 2012-03-05 05:47:39 PST
Created attachment 602861 [details] [diff] [review]
Removes nsCRT:: in nsCRT::strlen, changes to nsCRT.h and nsCRT.cpp, and addresses formatting issues
Comment 19 Serge Gautherie (:sgautherie) 2012-03-05 06:29:12 PST
Comment on attachment 602861 [details] [diff] [review]
Removes nsCRT:: in nsCRT::strlen, changes to nsCRT.h and nsCRT.cpp, and addresses formatting issues

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

::: xpcom/io/nsEscape.cpp
@@ +289,5 @@
>  nsEscapeHTML2(const PRUnichar *aSourceBuffer, PRInt32 aSourceBufferLen)
>  {
>    // if the caller didn't calculate the length
>    if (aSourceBufferLen < 0) {
> +    aSourceBufferLen = strlen(aSourceBuffer); // ...then I will

Nit: I suggest to also merge this comment with the one above, for example
"Calculate the length, if the caller didn't."
Comment 20 Shriram (irc: Mavericks) Away 2012-03-05 07:47:24 PST
Created attachment 602907 [details] [diff] [review]
Merges a comment in addition to changes from previous patch
Comment 21 Serge Gautherie (:sgautherie) 2012-03-05 08:05:14 PST
Comment on attachment 602907 [details] [diff] [review]
Merges a comment in addition to changes from previous patch

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

::: xpcom/io/nsEscape.cpp
@@ +287,5 @@
>  
>  PRUnichar *
>  nsEscapeHTML2(const PRUnichar *aSourceBuffer, PRInt32 aSourceBufferLen)
>  {
> +  // Calculate the length, if the caller didn't

Nit: you missed to add the point.

But no need for a new patch for that (either), and set r? flag again when you update a patch.
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-05 10:54:12 PST
Comment on attachment 602907 [details] [diff] [review]
Merges a comment in addition to changes from previous patch

I'm not the right person to review this. Perhaps try asking on #developers?
Comment 23 Doug Turner (:dougt) 2012-03-23 09:54:29 PDT
Comment on attachment 602907 [details] [diff] [review]
Merges a comment in addition to changes from previous patch

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

This is fine, but I really think we want to remove nsCRT::strlen() so that we don't have to do another removal of callers.  Or at least make that static method assert in debug so that devs can see their mistake.
Comment 24 Serge Gautherie (:sgautherie) 2012-03-23 16:56:05 PDT
(In reply to Doug Turner (:dougt) from comment #23)
> This is fine, but I really think we want to remove nsCRT::strlen() so that
> we don't have to do another removal of callers.  Or at least make that
> static method assert in debug so that devs can see their mistake.

I suggest to do it in a separate patch, fwiw.
Comment 25 Shriram (irc: Mavericks) Away 2012-04-02 20:06:52 PDT
Created attachment 611704 [details] [diff] [review]
Patch addresses comment #21 and removes nsCRT::strlen(const char*) occurrences
[Same as next attachment]
Comment 26 Shriram (irc: Mavericks) Away 2012-04-02 20:09:15 PDT
Created attachment 611705 [details] [diff] [review]
Patch addresses comment #21 and removes nsCRT::strlen(const char*) occurrences
[Backed out: comment 29]
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-04-03 06:30:21 PDT
Comment on attachment 611705 [details] [diff] [review]
Patch addresses comment #21 and removes nsCRT::strlen(const char*) occurrences
[Backed out: comment 29]

Just use checkin-needed
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-04-03 17:10:06 PDT
Thanks for the patch! Please keep your commit message consistent between patches, however. I had to dig through the patches to come up with one before checking in. Your commit message should just be a concise summary of the changes your patch is making.

https://hg.mozilla.org/integration/mozilla-inbound/rev/17deb5f61b4d
Comment 29 Ryan VanderMeulen [:RyanVM] 2012-04-03 17:25:38 PDT
Had to back this out due to build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/308440acc7b7

nsHashtable.cpp

e:/builds/moz2_slave/m-in-w32/build/xpcom/ds/nsHashtable.cpp(650) : error C2664: 'strlen' : cannot convert parameter 1 from 'const PRUnichar *' to 'const char *'

        Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

Please get someone to push your next patch to Try before setting checkin-needed.
http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound
Comment 30 Shriram (irc: Mavericks) Away 2012-04-04 00:27:34 PDT
(In reply to Ryan VanderMeulen from comment #29)
Owh, ok. will do that for next patch
Comment 31 Marco Bonardo [::mak] 2012-04-04 05:01:23 PDT
(In reply to Ryan VanderMeulen from comment #29)
> Please get someone to push your next patch to Try before setting
> checkin-needed.


Can use [autoland] for try: https://wiki.mozilla.org/Build:Autoland
Comment 32 Justin Lebar (not reading bugmail) 2012-04-04 06:16:21 PDT
Okay, but before we land this, we want a patch which removes nsCRT::strlen, right?  I'd like both of them to land at the same time, so we can finally kill this thing.
Comment 33 Serge Gautherie (:sgautherie) 2012-04-04 08:10:46 PDT
(In reply to Ryan VanderMeulen from comment #29)
> e:/builds/moz2_slave/m-in-w32/build/xpcom/ds/nsHashtable.cpp(650) : error
> C2664: 'strlen' : cannot convert parameter 1 from 'const PRUnichar *' to
> 'const char *'

I hadn't noticed, but there is actually 2 nsCRT::strlen(...):
http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsCRT.h
{
129   static PRUint32 strlen(const char* s) {                                       
130     return PRUint32(::strlen(s));                                               
131   }                                                                             

198   static PRUint32 strlen(const PRUnichar* s) {
199     // XXXbsmedberg: remove this null-check at some point
200     if (!s) {
201       NS_ERROR("Passing null to nsCRT::strlen");
202       return 0;
203     }
204     return NS_strlen(s);
205   }
}

This bug, as filed, is about removing the first/deprecated method only...
Let's have a patch that (fully) does that part only/first.

As the first method actually calls the second one, do we care about loosing the null-check?
Benjamin, would it even be alright to remove it from the second method (now)?
Comment 34 Serge Gautherie (:sgautherie) 2012-04-04 08:14:58 PDT
(In reply to Serge Gautherie (:sgautherie) from comment #33)
> 130     return PRUint32(::strlen(s));                                       
> As the first method actually calls the second one

Ah, I have a doubt: is ::strlen() rather the libc function?
Comment 35 Benjamin Smedberg [:bsmedberg] 2012-04-04 08:30:31 PDT
Yes, ::strlen is the libc function. And yes, they both ought to be removed.
Comment 36 Shriram (irc: Mavericks) Away 2012-04-04 21:19:58 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #35)
> Yes, ::strlen is the libc function. And yes, they both ought to be removed.

If the first, deprecated method's removed and second method doesn't have null check,  is the null check handled prior or taken care of elsewhere ?
Comment 37 Shriram (irc: Mavericks) Away 2012-04-07 06:55:29 PDT
Created attachment 613100 [details] [diff] [review]
Patch that removes nsCRT::strlen(const char *) from nsCRT.h and removes null check from nsCRT::strlen(const PRUnichar*)
[Has obsolete f= and r=]

Should the null check removal be done in a separate patch ?
Comment 38 Shriram (irc: Mavericks) Away 2012-04-07 06:58:11 PDT
Created attachment 613101 [details] [diff] [review]
Patch that removes nsCRT::strlen(const char *) from nsCRT.h and removes null check from nsCRT::strlen(const PRUnichar*)
Comment 39 Serge Gautherie (:sgautherie) 2012-04-07 07:28:41 PDT
Comment on attachment 613101 [details] [diff] [review]
Patch that removes nsCRT::strlen(const char *) from nsCRT.h and removes null check from nsCRT::strlen(const PRUnichar*)

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

(In reply to Serge Gautherie (:sgautherie) from comment #33)
> Let's have a patch that (fully) does that part only/first.

(In reply to Shriram (irc: Mavericks) from comment #37)
> Should the null check removal be done in a separate patch ?

Yes, split your current patch in two: one for char (now), one for PRUnichar (later).

***

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #35)
> yes, they both ought to be removed.

Could you give an example code to replace 'strlen(const PRUnichar* s)' calls?

::: xpcom/ds/nsCRT.h
@@ +124,5 @@
>  
>    /** Compute the string length of s
>     @param s the string in question
>     @return the length of s
>     */

Remove the comments related to this function too.
Comment 40 Serge Gautherie (:sgautherie) 2012-04-07 07:32:29 PDT
(In reply to Serge Gautherie (:sgautherie) from comment #39)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #35)
> > yes, they both ought to be removed.
> 
> Could you give an example code to replace 'strlen(const PRUnichar* s)' calls?

Ah, what was your 'both' referring to? Both strlen() functions or both 'char function and PRUnichar null check (only)'?
Comment 41 Shriram (irc: Mavericks) Away 2012-04-07 18:13:10 PDT
Created attachment 613149 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h
Comment 42 Serge Gautherie (:sgautherie) 2012-04-08 07:54:02 PDT
Comment on attachment 613149 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h

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

Use "Bug 150073. Remove 'nsCRT::strlen(const char* s)'."

::: xpcom/ds/nsCRT.h
@@ +117,5 @@
>     ***  Additionally, the following char* string utilities
>     ***  are no longer supported, please use the
>     ***  corresponding lib C functions instead.
>     ***
>     ***  nsCRT::strlen()

Remove these comment lines too!
Comment 43 Shriram (irc: Mavericks) Away 2012-04-08 09:01:55 PDT
Created attachment 613182 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h

removed the comments, as per comment #42, in this patch.
Comment 44 Mozilla RelEng Bot 2012-04-08 12:17:26 PDT
Try run for f99d6e23d2ba is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f99d6e23d2ba
Results (out of 15 total builds):
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-f99d6e23d2ba
Comment 45 :Ms2ger (⌚ UTC+1/+2) 2012-04-08 12:33:26 PDT
(In reply to Serge Gautherie (:sgautherie) from comment #39)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #35)
> > yes, they both ought to be removed.
> 
> Could you give an example code to replace 'strlen(const PRUnichar* s)' calls?

NS_strlen(s)?
Comment 46 Ryan VanderMeulen [:RyanVM] 2012-04-08 15:23:00 PDT
(In reply to Mozilla RelEng Bot from comment #44)
> Try run for f99d6e23d2ba is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=f99d6e23d2ba
> Results (out of 15 total builds):
>     failure: 15
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-
> f99d6e23d2ba

Same failure as comment #29.
Comment 47 :Ms2ger (⌚ UTC+1/+2) 2012-04-09 00:18:41 PDT
(In reply to Ryan VanderMeulen from comment #46)
> (In reply to Mozilla RelEng Bot from comment #44)
> > Try run for f99d6e23d2ba is complete.
> > Detailed breakdown of the results available here:
> >     https://tbpl.mozilla.org/?tree=Try&rev=f99d6e23d2ba
> > Results (out of 15 total builds):
> >     failure: 15
> > Builds (or logs if builds failed) available at:
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-
> > f99d6e23d2ba
> 
> Same failure as comment #29.

Unsurprisingly, as there is no strlen(const PRUnichar*), yet the patch tries to call that.
Comment 48 Shriram (irc: Mavericks) Away 2012-04-10 08:16:15 PDT
Created attachment 613612 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h

Re-added the nsCRT:: for nsCRT::strlen(const PRUnichar*) lines
Removal of it will be addressed @ Bug 743581
Comment 49 Serge Gautherie (:sgautherie) 2012-04-20 16:27:55 PDT
Comment on attachment 613612 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h

>diff --git a/xpcom/ds/nsHashtable.cpp b/xpcom/ds/nsHashtable.cpp
>--- a/xpcom/ds/nsHashtable.cpp
>+++ b/xpcom/ds/nsHashtable.cpp
>@@ -641,18 +641,18 @@ nsStringKey::nsStringKey(const nsAString
>+        mStrLen = nsCRT::strlen(str);
>     if (mStrLen == PRUint32(-1))
>-        mStrLen = nsCRT::strlen(str);

(At least) This is wrong.

*****

Please, submit your next patch to Try.
Comment 50 Shriram (irc: Mavericks) Away 2012-04-20 18:47:22 PDT
(In reply to Serge Gautherie (:sgautherie) from comment #49)
> Comment on attachment 613612 [details] [diff] [review]
> Patch that removes function nsCRT::strlen(const char *) from nsCRT.h
> 
> >diff --git a/xpcom/ds/nsHashtable.cpp b/xpcom/ds/nsHashtable.cpp
> >--- a/xpcom/ds/nsHashtable.cpp
> >+++ b/xpcom/ds/nsHashtable.cpp
> >@@ -641,18 +641,18 @@ nsStringKey::nsStringKey(const nsAString
> >+        mStrLen = nsCRT::strlen(str);
> >     if (mStrLen == PRUint32(-1))
> >-        mStrLen = nsCRT::strlen(str);
> 
> (At least) This is wrong.
> 
> *****
> 
> Please, submit your next patch to Try.

Aah, I thought the patch applied cleanly after pulling the updates, because there were no conflicts. I wil address this issue, and submit next patch to Try.
Comment 51 Shriram (irc: Mavericks) Away 2012-04-27 06:24:53 PDT
Created attachment 619007 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h

I've received https://tbpl.mozilla.org/?tree=Try&rev=173495d3bb7f 
for latest patch.
Comment 52 Doug Turner (:dougt) 2012-04-27 08:45:17 PDT
Comment on attachment 619007 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h

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

::: intl/unicharutil/src/nsSaveAsCharset.cpp
@@ +215,2 @@
>  
>    // estimate and allocate the target buffer (reserve extra memory for fallback)

Changes to this file should be dropped.  It looks like the orignal file had the c++ comments alligned.

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

remove nsCRT::?

::: layout/printing/nsPrintEngine.cpp
@@ +2413,5 @@
>  {
>    // Make sure the URLS don't get too long for the progress dialog
>    if (aStr && nsCRT::strlen(aStr) > aLen) {
>      if (aDoFront) {
> +      PRUnichar * ptr = &aStr[nsCRT::strlen(aStr) - aLen + 3];

Remove nsCRT::?

::: netwerk/streamconv/converters/mozTXTToHTMLConv.cpp
@@ +742,5 @@
>  {
>    if ( !aInString || !tagTXT || !imageName )
>        return false;
>  
> +  PRInt32  tagLen = strlen(tagTXT);

good.  but drop the extra space better PRInt32 and tagLen

::: widget/cocoa/nsPrintDialogX.mm
@@ +85,5 @@
>    PRUnichar** docTitles;
>    PRUint32 titleCount;
>    nsresult rv = aWebBrowserPrint->EnumerateDocumentNames(&titleCount, &docTitles);
>    if (NS_SUCCEEDED(rv) && titleCount > 0) {
> +    CFStringRef cfTitleString = CFStringCreateWithCharacters(NULL, docTitles[0],  nsCRT::strlen(docTitles[0]));

drop the nsCRT:: ?

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

drop the nsCRT:: ?

::: xpcom/io/nsEscape.cpp
@@ +292,2 @@
>    if (aSourceBufferLen < 0) {
> +    aSourceBufferLen = nsCRT::strlen(aSourceBuffer);

Same?
Comment 53 :Ms2ger (⌚ UTC+1/+2) 2012-04-27 09:47:23 PDT
(In reply to Doug Turner (:dougt) from comment #52)
> ::: layout/base/nsBidi.cpp
> @@ +278,5 @@
> >      return NS_ERROR_INVALID_ARG;
> >    }
> >  
> >    if(aLength==-1) {
> > +    aLength = nsCRT::strlen(aText);
> 
> remove nsCRT::?

No. There is no strlen(const PRUnichar*).
Comment 54 Shriram (irc: Mavericks) Away 2012-04-28 01:23:22 PDT
Created attachment 619261 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h

Fixed the nits
Comment 55 Doug Turner (:dougt) 2012-05-01 08:37:30 PDT
Comment on attachment 619261 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h

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

basically, i don't want you to make any white space changes in the next patch unless you are removing the nsCRT prefix.  It's easier to read.  If you remove those from your patch, r+.

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

don't even bother with this change until you drop nsCRT::strlen

::: layout/printing/nsPrintEngine.cpp
@@ +2413,5 @@
>  {
>    // Make sure the URLS don't get too long for the progress dialog
>    if (aStr && nsCRT::strlen(aStr) > aLen) {
>      if (aDoFront) {
> +      PRUnichar * ptr = &aStr[nsCRT::strlen(aStr) - aLen + 3];

same.

::: 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, nsCRT::strlen(indexString));

same.

::: widget/cocoa/nsPrintDialogX.mm
@@ +85,5 @@
>    PRUnichar** docTitles;
>    PRUint32 titleCount;
>    nsresult rv = aWebBrowserPrint->EnumerateDocumentNames(&titleCount, &docTitles);
>    if (NS_SUCCEEDED(rv) && titleCount > 0) {
> +    CFStringRef cfTitleString = CFStringCreateWithCharacters(NULL, docTitles[0],  nsCRT::strlen(docTitles[0]));

same.
Comment 56 Shriram (irc: Mavericks) Away 2012-05-01 13:07:34 PDT
Created attachment 620039 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h

This patch addresses white space changes in last comment.
Comment 57 Serge Gautherie (:sgautherie) 2012-05-02 00:12:40 PDT
Comment on attachment 620039 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h

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

::: xpcom/io/nsEscape.cpp
@@ +287,5 @@
>  
>  PRUnichar *
>  nsEscapeHTML2(const PRUnichar *aSourceBuffer, PRInt32 aSourceBufferLen)
>  {
> +  // Calculate the length, if the caller didn't.

Nit: this change(s) didn't really belong to this patch, but that will do.
Comment 58 Ryan VanderMeulen [:RyanVM] 2012-05-03 03:53:32 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/82ad89c00ec6
Comment 59 Ed Morley [:emorley] 2012-05-04 03:38:32 PDT
https://hg.mozilla.org/mozilla-central/rev/82ad89c00ec6

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