Closed
Bug 898691
Opened 11 years ago
Closed 11 years ago
Wasted work in TestTextFormatter.cpp:main()
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: pchang9, Assigned: pchang9)
Details
(Keywords: perf)
Attachments
(1 file, 4 obsolete files)
|
1.06 KB,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 (Beta/Release) Build ID: 20130116073211 Steps to reproduce: The problem appears in changeset 138350:18467a85acf6. I have attached a simple one-line patch that fixes it. In method main() in xpcom/tests/TestTextFormatter.cpp, the loop on line 32 should break immediately after "test_ok" is set to "false". All the iterations after "test_ok" is set to "false" do not perform any useful work, at best they just set "test_ok" again to "false". Expected results: Break loop after "test_ok" is set to "false".
|
||
Updated•11 years ago
|
Component: Untriaged → XPCOM
Product: Firefox → Core
Attachment #784186 -
Flags: review?(benjamin)
Attachment #782047 -
Attachment is obsolete: true
Attachment #784186 -
Attachment is obsolete: true
Attachment #784186 -
Flags: review?(benjamin)
Attachment #785389 -
Flags: review?(justin.lebar+bug)
Attachment #785389 -
Flags: review?(benjamin)
|
||
Comment 3•11 years ago
|
||
Comment on attachment 785389 [details] [diff] [review] 898691.patch While we're here, could you please brace the for loop?
Attachment #785389 -
Flags: review?(justin.lebar+bug)
Attachment #785389 -
Flags: review?(benjamin)
Attachment #785389 -
Attachment is obsolete: true
Attachment #785419 -
Flags: review?(justin.lebar+bug)
|
|
||
Comment 5•11 years ago
|
||
Comment on attachment 785419 [details] [diff] [review] 898691-1.patch Looks good to me. Thanks for the patch! The only other thing you need to do is to change the patch description, and then set checkin-needed in the bug's keyword field. Then someone will check this patch in within the next few days. > Bug 898691 (rev #1) - Break loop after "test_ok" is set to "false". Also brace the for loop. Please remove "(rev #1)" and add "r=jlebar" at the end. Also, we try to have patch descriptions say why the patch is doing something; that's more important than describing every little thing we changed, since you can always look at the diff for that. Something like "Avoid wasted work in TestTextFormatter.cpp" would be good, IMO.
Attachment #785419 -
Flags: review?(justin.lebar+bug) → review+
Attachment #785419 -
Attachment is obsolete: true
Keywords: checkin-needed
|
||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bded8515b5db
Assignee: nobody → pchang9
Keywords: checkin-needed
|
|
||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bded8515b5db
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in
before you can comment on or make changes to this bug.
Description
•