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.

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 :-)

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

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?

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

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.

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! )

Related: bug 276845.

"Found 52 matching lines in 27 files"

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?

(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 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?

Includes all matching files now.

Has an accurate summary of changes message in the patch.

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 '*'.

Owh! yes, i shouldn't have touched nsCRT.h due to comment above it.

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 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 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 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.

(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 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

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

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

(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)?

(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.

(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 ?

Should the null check removal be done in a separate patch ?

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.

(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 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!

removed the comments, as per comment #42, in this patch.

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.

Re-added the nsCRT:: for nsCRT::strlen(const PRUnichar*) lines Removal of it will be addressed @ Bug 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.

(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.

I've received https://tbpl.mozilla.org/?tree=Try&rev=173495d3bb7f for latest patch.

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?

(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*).

Fixed the nits

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.

This patch addresses white space changes in last comment.

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.