nsCRT::strlen(const char *) should go away

RESOLVED FIXED in mozilla15

Status

()

Core
XPCOM
--
trivial
RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: dmose, Assigned: Shriram (irc: Mavericks))

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 15 obsolete attachments)

8.19 KB, patch
dougt
: review+
sgautherie
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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

15 years ago
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 :-)
(Reporter)

Comment 2

15 years ago
Thomas: why add an extra intermediate step here -- ie why not just get rid of it
entirely?

Comment 3

15 years ago
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?
(Reporter)

Comment 4

15 years ago
CVS generally handles merges pretty well, I don't think that sort of problem is
likely to be an issue.

Comment 5

15 years ago
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.

Updated

15 years ago
Blocks: 124536
Keywords: perf

Comment 6

15 years ago
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! )

Assignee: cathleennscp → nobody
QA Contact: scc → xpcom

Comment 7

11 years ago
Related: bug 276845.
"Found 52 matching lines in 27 files"
Depends on: 715938
(Assignee)

Comment 9

5 years ago
Created attachment 598526 [details] [diff] [review]
Patch that removes nsCRT:: for nsCRT::strlen
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?
Assignee: nobody → kshriram18
Severity: normal → trivial
Status: NEW → ASSIGNED
Flags: in-testsuite-
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 602587 [details] [diff] [review]
Removes nsCRT:: in nsCRT::strlen
Attachment #598526 - Attachment is obsolete: true
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?
(Assignee)

Comment 14

5 years ago
Created attachment 602684 [details] [diff] [review]
Removes nsCRT:: in nsCRT::strlen

Includes all matching files now.
Attachment #602587 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 602685 [details] [diff] [review]
Removes nsCRT:: in nsCRT::strlen

Has an accurate summary of changes message in the patch.
Attachment #602684 - Attachment is obsolete: true
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 '*'.
Attachment #602685 - Flags: feedback-
(Assignee)

Comment 17

5 years ago
Owh! yes, i shouldn't have touched nsCRT.h due to comment above it.
(Assignee)

Comment 18

5 years ago
Created attachment 602861 [details] [diff] [review]
Removes nsCRT:: in nsCRT::strlen, changes to nsCRT.h and nsCRT.cpp, and addresses formatting issues
Attachment #602685 - Attachment is obsolete: true
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."
Attachment #602861 - Flags: review?(gavin.sharp)
Attachment #602861 - Flags: feedback+
(Assignee)

Comment 20

5 years ago
Created attachment 602907 [details] [diff] [review]
Merges a comment in addition to changes from previous patch
Attachment #602861 - Attachment is obsolete: true
Attachment #602861 - Flags: review?(gavin.sharp)
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.
Attachment #602907 - Flags: review?(gavin.sharp)
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?
Attachment #602907 - Flags: review?(gavin.sharp)
Attachment #602907 - Flags: review?(doug.turner)
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.
Attachment #602907 - Flags: review?(doug.turner) → review+
(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.
(Assignee)

Comment 25

5 years ago
Created attachment 611704 [details] [diff] [review]
Patch addresses comment #21 and removes nsCRT::strlen(const char*) occurrences
[Same as next attachment]
Attachment #602907 - Attachment is obsolete: true
Attachment #611704 - Flags: checkin?
(Assignee)

Comment 26

5 years ago
Created attachment 611705 [details] [diff] [review]
Patch addresses comment #21 and removes nsCRT::strlen(const char*) occurrences
[Backed out: comment 29]
Attachment #611704 - Attachment is obsolete: true
Attachment #611705 - Flags: review+
Attachment #611705 - Flags: feedback+
Attachment #611705 - Flags: checkin?
Attachment #611704 - Flags: checkin?
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
Attachment #611705 - Flags: checkin?
Keywords: checkin-needed
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
Keywords: checkin-needed
Target Milestone: --- → mozilla14
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
Attachment #611704 - Attachment description: Patch addresses comment #21 and removes nsCRT::strlen(const char*) occurrences → Patch addresses comment #21 and removes nsCRT::strlen(const char*) occurrences [Same as next attachment]
(Assignee)

Comment 30

5 years ago
(In reply to Ryan VanderMeulen from comment #29)
Owh, ok. will do that for next patch
(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
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.
(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)?
Depends on: 337730
Attachment #611705 - Attachment description: Patch addresses comment #21 and removes nsCRT::strlen(const char*) occurrences → Patch addresses comment #21 and removes nsCRT::strlen(const char*) occurrences [Backed out: comment 29]
Attachment #611705 - Attachment is obsolete: true
Attachment #611705 - Flags: review+
Attachment #611705 - Flags: feedback+
(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?
Yes, ::strlen is the libc function. And yes, they both ought to be removed.
(Assignee)

Comment 36

5 years ago
(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 ?
(Assignee)

Comment 37

5 years ago
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 ?
Attachment #613100 - Flags: feedback?(sgautherie.bz)
(Assignee)

Comment 38

5 years ago
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*)
Attachment #613100 - Attachment is obsolete: true
Attachment #613101 - Flags: feedback?(sgautherie.bz)
Attachment #613100 - Flags: feedback?(sgautherie.bz)
Attachment #613100 - Attachment description: Patch that removes nsCRT::strlen(const char *) from nsCRT.h and removes null check from nsCRT::strlen(const PRUnichar*) → Patch that removes nsCRT::strlen(const char *) from nsCRT.h and removes null check from nsCRT::strlen(const PRUnichar*) [Has obsolete f= and r=]
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.
Attachment #613101 - Flags: feedback?(sgautherie.bz) → feedback-
(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)'?
(Assignee)

Comment 41

5 years ago
Created attachment 613149 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h
Attachment #613101 - Attachment is obsolete: true
Attachment #613149 - Flags: feedback?(sgautherie.bz)
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!
Attachment #613149 - Flags: feedback?(sgautherie.bz) → feedback-
(Assignee)

Comment 43

5 years ago
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.
Attachment #613149 - Attachment is obsolete: true
Attachment #613182 - Flags: feedback?(sgautherie.bz)

Comment 44

5 years ago
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
(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)?
(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.
(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.
(Assignee)

Comment 48

5 years ago
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
Attachment #613182 - Attachment is obsolete: true
Attachment #613612 - Flags: feedback?(sgautherie.bz)
Attachment #613182 - Flags: feedback?(sgautherie.bz)
Blocks: 743581
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.
Attachment #613612 - Flags: feedback?(sgautherie.bz) → review-
(Assignee)

Comment 50

5 years ago
(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.
(Assignee)

Comment 51

5 years ago
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.
Attachment #613612 - Attachment is obsolete: true
Attachment #619007 - Flags: review?(doug.turner)
Attachment #619007 - Flags: feedback?(sgautherie.bz)
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?
Attachment #619007 - Flags: review?(doug.turner) → review-
(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*).
(Assignee)

Comment 54

5 years ago
Created attachment 619261 [details] [diff] [review]
Patch that removes function nsCRT::strlen(const char *) from nsCRT.h

Fixed the nits
Attachment #619007 - Attachment is obsolete: true
Attachment #619261 - Flags: review?(doug.turner)
Attachment #619007 - Flags: feedback?(sgautherie.bz)
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.
Attachment #619261 - Flags: review?(doug.turner) → review-
(Assignee)

Comment 56

5 years ago
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.
Attachment #619261 - Attachment is obsolete: true
Attachment #620039 - Flags: review?(doug.turner)
Attachment #620039 - Flags: feedback?(doug.turner)

Updated

5 years ago
Attachment #620039 - Flags: review?(doug.turner)
Attachment #620039 - Flags: review+
Attachment #620039 - Flags: feedback?(doug.turner)
Attachment #620039 - Flags: feedback+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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.
Attachment #620039 - Flags: feedback+ → feedback+
https://hg.mozilla.org/integration/mozilla-inbound/rev/82ad89c00ec6
Keywords: checkin-needed
Target Milestone: mozilla14 → mozilla15
https://hg.mozilla.org/mozilla-central/rev/82ad89c00ec6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
No longer depends on: 337730
You need to log in before you can comment on or make changes to this bug.