Double var assign in QCache.cpp

RESOLVED FIXED in Future

Status

--
minor
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: y-a0, Assigned: lhansen)

Tracking

unspecified
Future

Details

(Whiteboard: Has patch)

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
User-Agent:       Opera/9.80 (Windows NT 5.1; U; ru) Presto/2.2.15 Version/10.00
Build Identifier: 

patch:

diff --git a/core/QCache.cpp b/core/QCache.cpp
--- a/core/QCache.cpp
+++ b/core/QCache.cpp
@@ -99,7 +99,7 @@
 			}
 			if (evicted_prev)
 			{
-				evicted = evicted_prev->next;
+//				evicted = evicted_prev->next;
 				AvmAssert(evicted != NULL && evicted != gen);
 				WB(m_gc, evicted_prev, &evicted_prev->next, evicted->next);
 			}

Reproducible: Always

Comment 1

9 years ago
Could you explain a bit more? What is the nature of the bug? Inspection doesn't reveal it to me.
(Reporter)

Comment 2

9 years ago
In this code one value is twice assigned to a variable "evicted". I have thought that it can influence performance. I am not assured completely, I have not enough experience with C++.
(Assignee)

Comment 3

9 years ago
There's a flaw in the code but the offered patch isn't the optimal one.  If you look at the eviction code, 'evicted' is set before the break in the scanning loop.  But then both arms of the 'if' following the loop set 'evicted' first thing, so the assignment in the loop is actually dead and can be removed.

(I expect most compilers will remove the redundant assignment, though, it should be easy enough.)

Comment 4

9 years ago
Ah. Gotcha. Yeah, a minimal inefficiency, but worth correcting next time we touch this code.

Updated

9 years ago
Priority: -- → P4
Target Milestone: --- → Future
(Assignee)

Updated

9 years ago
Priority: P4 → --
(Assignee)

Updated

9 years ago
Assignee: nobody → lhansen
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 5

9 years ago
Created attachment 438378 [details] [diff] [review]
Obvious patch
Attachment #438378 - Flags: review?(stejohns)
(Assignee)

Updated

9 years ago
Whiteboard: Has patch

Updated

9 years ago
Attachment #438378 - Flags: review?(stejohns) → review+
(Assignee)

Comment 6

9 years ago
tamarin-redux changeset:   4399:7d02e45ab981
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.