Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(146)

Issue 6265002: Adding SearchableList for implementing a list with fast search capability. (Closed)

Created:
9 years, 11 months ago by marklam
Modified:
9 years, 7 months ago
CC:
v8-dev
Visibility:
Public.

Description

Adding SearchableList for implementing a list with fast search capability.

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 8

Patch Set 3 : '' #

Patch Set 4 : '' #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+109 lines, -0 lines) Patch
M src/globals.h View 1 2 3 1 chunk +1 line, -0 lines 1 comment Download
M src/list.h View 1 2 2 chunks +49 lines, -0 lines 0 comments Download
M src/list-inl.h View 1 2 1 chunk +59 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
marklam
9 years, 11 months ago (2011-01-13 20:46:26 UTC) #1
marklam
Mikhail, FYI, I just got some feedback from Søren on the list implementation and other ...
9 years, 11 months ago (2011-01-14 00:16:32 UTC) #2
marklam
Mikhail, Søren, I've incorporated Søren's suggested changes. Please take a look. Thanks.
9 years, 11 months ago (2011-01-18 02:50:51 UTC) #3
Søren Thygesen Gjesse
LGTM, with comments addressed. http://codereview.chromium.org/6265002/diff/6001/src/list-inl.h File src/list-inl.h (right): http://codereview.chromium.org/6265002/diff/6001/src/list-inl.h#newcode226 src/list-inl.h:226: bool SearchableList<T, P>::Contains(const T& elm) ...
9 years, 11 months ago (2011-01-18 07:44:40 UTC) #4
marklam
Hi Søren, I've made the necessary changes and addressed your comments. I've also added the ...
9 years, 11 months ago (2011-01-18 22:33:16 UTC) #5
Søren Thygesen Gjesse
9 years, 11 months ago (2011-01-19 07:43:04 UTC) #6
When scanning through the lol change I did see bsearch used directly, but did
not think further about that.

Anyway if SearchableList will not be used I don't think we should add it to the
codebase. You can just revive this change if needed at some later point.

Just for the record there should also add some tests to test-list.cc as well
(sorry for not mentioning that earlier).

Closing the issue for now.

http://codereview.chromium.org/6265002/diff/13001/src/globals.h
File src/globals.h (right):

http://codereview.chromium.org/6265002/diff/13001/src/globals.h#newcode322
src/globals.h:322: template <typename T, class P = FreeStoreAllocationPolicy>
class BSearchList;
This should be SearchableList

Powered by Google App Engine
This is Rietveld 408576698