Last Comment Bug 745659 - Remove obsolete nsPrintfCString constructor which takes a length
: Remove obsolete nsPrintfCString constructor which takes a length
Status: RESOLVED FIXED
[mentor=jlebar][lang=c++]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Patrick
:
Mentors:
Depends on: 743056
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-15 18:53 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-04-25 07:35 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Removes the nsPrintfCString constructor which takes a length and all subsequent calls (26.37 KB, patch)
2012-04-22 11:27 PDT, Patrick
justin.lebar+bug: review-
Details | Diff | Review
New patch that fixes the errors reported by the build failure report (36.89 KB, patch)
2012-04-23 14:45 PDT, Patrick
justin.lebar+bug: review+
Details | Diff | Review
Bug-745659 - Patch 3 (61.83 KB, patch)
2012-04-24 13:02 PDT, Patrick
justin.lebar+bug: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-04-15 18:53:33 PDT
We should just remove this constructor and all the code which calls it.
Comment 1 Patrick 2012-04-22 11:27:54 PDT
Created attachment 617332 [details] [diff] [review]
Removes the nsPrintfCString constructor which takes a length and all subsequent calls

The changes were able to compile. No new tests written because I only removed code.
Comment 2 Justin Lebar (not reading bugmail) 2012-04-22 19:54:10 PDT
\o/

This looks good.  I'm going to run it through tryserver just to make sure we're not missing something on a platform you didn't test on.  The live results are on tbpl [1], and the bot should post a comment in this bug when the tests complete.

These first bugs are also about figuring out mechanics, so I should mention: This is kind of a weird looking patch.  It looks like maybe you concatenated the result of a few hg export's (perhaps hg did that for you?).  We usually look for a patch which has just one "hg export" header.

In that header, you'll want to make the commit message take the form

  "Bug 745659 - What was done to fix the bug. r=jlebar"

Once this patch passes the tests and you upload a patch with the right commit header, I'll check it in for you!

(bsmedberg, I assume you're OK with me reviewing this?)
Comment 3 Justin Lebar (not reading bugmail) 2012-04-22 19:54:43 PDT
er, and the tbpl link:

[1] https://tbpl.mozilla.org/?tree=Try&rev=e3028a232847
Comment 4 Justin Lebar (not reading bugmail) 2012-04-22 20:06:33 PDT
Comment on attachment 617332 [details] [diff] [review]
Removes the nsPrintfCString constructor which takes a length and all subsequent calls

r- because this is burning Linux and MacOS on try. 

If you want to be able to push directly to try, we can get that set up for you.  You need L1 access, and I can vouch for you.

http://www.mozilla.org/hacking/commit-access-policy/
Comment 5 Mozilla RelEng Bot 2012-04-23 01:16:22 PDT
Try run for e3028a232847 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e3028a232847
Results (out of 119 total builds):
    success: 91
    warnings: 19
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-e3028a232847
Comment 6 Benjamin Smedberg [:bsmedberg] 2012-04-23 06:25:14 PDT
jlebar, yes please review away, I went ahead and added you as an official peer of the String module also.
Comment 7 Patrick 2012-04-23 14:45:39 PDT
Created attachment 617651 [details] [diff] [review]
New patch that fixes the errors reported by the build failure report

I looked at the build failure report and was able to locate and fix those errors. I cleaned the build directory and everything recompiles without error. Attached is the new patch. Sorry about the inconvenience.
Comment 8 Justin Lebar (not reading bugmail) 2012-04-23 14:51:05 PDT
The hg headers in the new diff look good; thanks.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ce5c67c21fec
Comment 9 Justin Lebar (not reading bugmail) 2012-04-23 22:38:01 PDT
Comment on attachment 617651 [details] [diff] [review]
New patch that fixes the errors reported by the build failure report

>diff -r df1bdf389d39 -r 6514ffcd91f5 netwerk/ipc/NeckoCommon.h
>--- a/netwerk/ipc/NeckoCommon.h	Sun Apr 22 12:07:59 2012 -0400
>+++ b/netwerk/ipc/NeckoCommon.h	Sun Apr 22 12:14:36 2012 -0400
>@@ -74,7 +74,7 @@
> 
> #define DROP_DEAD()                                                            \
>   do {                                                                         \
>-    nsPrintfCString msg(1000,"NECKO ERROR: '%s' UNIMPLEMENTED",                \
>+    nsPrintfCString msg("NECKO ERROR: '%s' UNIMPLEMENTED",                \
>                         __FUNCTION__);                                         \
>     NECKO_MAYBE_ABORT(msg);                                                    \
>     return NS_ERROR_NOT_IMPLEMENTED;                                           \
>@@ -82,7 +82,7 @@
> 
> #define ENSURE_CALLED_BEFORE_ASYNC_OPEN()                                      \
>   if (mIsPending || mWasOpened) {                                              \
>-    nsPrintfCString msg(1000, "'%s' called after AsyncOpen: %s +%d",           \
>+    nsPrintfCString msg("'%s' called after AsyncOpen: %s +%d",           \
>                         __FUNCTION__, __FILE__, __LINE__);                     \
>     NECKO_MAYBE_ABORT(msg);                                                    \
>   }                                                                            \

Would you mind if I was really anal and asked you to fix this whitespace?

Also, in the future, please attach -U8 -p diffs.  I have in my hgrc:

  [diff]
  git = 1
  showfunc = 1
  unified = 8

Looks good aside from that, and looks good on try.  If you fix the whitespace, I'll check this in.
Comment 10 Mozilla RelEng Bot 2012-04-24 01:15:22 PDT
Try run for ce5c67c21fec is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ce5c67c21fec
Results (out of 218 total builds):
    exception: 1
    success: 181
    warnings: 36
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-ce5c67c21fec
Comment 11 Patrick 2012-04-24 13:02:52 PDT
Created attachment 618003 [details] [diff] [review]
Bug-745659 - Patch 3

Removes the nsPrintfCString constructor which takes a length and all subsequent calls. It also fixes the extra spaces issue.
Comment 12 Justin Lebar (not reading bugmail) 2012-04-24 18:12:23 PDT
Comment on attachment 618003 [details] [diff] [review]
Bug-745659 - Patch 3

Pushed to mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/da03b8dcabe8

Congratulations on your first patch!
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-25 07:35:30 PDT
https://hg.mozilla.org/mozilla-central/rev/da03b8dcabe8

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