Wasted work in TestTextFormatter.cpp:main()

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: pchang9, Assigned: pchang9)

Tracking

({perf})

Trunk
mozilla26
x86_64
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Posted file Suggested patch (obsolete) —
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".
Keywords: perf
OS: Windows 7 → All
Component: Untriaged → XPCOM
Product: Firefox → Core
Posted patch 898691.patch (obsolete) — Splinter Review
Attachment #784186 - Flags: review?(benjamin)
Posted patch 898691.patch (obsolete) — Splinter Review
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 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)
Posted patch 898691-1.patch (obsolete) — Splinter Review
Attachment #785389 - Attachment is obsolete: true
Attachment #785419 - Flags: review?(justin.lebar+bug)
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
https://hg.mozilla.org/mozilla-central/rev/bded8515b5db
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.