Closed
Bug 391141
Opened 17 years ago
Closed 17 years ago
trace-malloc doesn't compile on VC8
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha8
People
(Reporter: dbaron, Assigned: dbaron)
Details
Attachments
(2 files)
(deleted),
patch
|
brendan
:
review+
brendan
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
brendan
:
review+
sayrer
:
approval1.9+
|
Details | Diff | Splinter Review |
trace-malloc doesn't compile on VC8 because one place in the Windows-specific code depends on incorrect C++ for-loop scoping. This patch fixes it.
I'd appreciate just a sanity check that this is correct; approval at the same time would be appreciated. (I suppose I could squeeze a PR_ASSERT into the for loop's condition with the , operator, but that seems even uglier.)
Attachment #275482 -
Flags: review?(brendan)
Comment 1•17 years ago
|
||
Comment on attachment 275482 [details] [diff] [review]
patch
Even better would be to rewrite using double indirection to unify the basis and inductive cases.
/be
Attachment #275482 -
Flags: review?(brendan)
Attachment #275482 -
Flags: review+
Assignee | ||
Comment 2•17 years ago
|
||
Checked in to trunk, 2007-08-10 14:26 -0700; I'm not going to worry about rewriting, although it is pretty simple...
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 3•17 years ago
|
||
This addresses comment 1. I've had it in my tree for a while (although I just fixed a mistake in what I had after testing it for the first time).
Attachment #281565 -
Flags: review?(brendan)
Comment 4•17 years ago
|
||
Comment on attachment 281565 [details] [diff] [review]
address review comment
ok by me, but the comma expression as condition is a bit hard on the eyes. But fixing that seems to require a while loop. Alternative: lose the non-null assert, since a null pointer deref is just as good in my book as an assertion (clean, unexploitable crash pointing to the offending code in this case, or as close to the offending code as the PR_ASSERT in this patch points).
/be
Attachment #281565 -
Flags: review?(brendan) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #281565 -
Flags: approval1.9?
Assignee | ||
Comment 5•17 years ago
|
||
Above patch checked in to trunk.
Updated•17 years ago
|
Attachment #281565 -
Flags: approval1.9? → approval1.9+
Comment hidden (off-topic) |
Comment hidden (off-topic) |
You need to log in
before you can comment on or make changes to this bug.
Description
•