LGTM with a few comments:
http://codereview.chromium.org/140056/diff/1/4 File src/compilation-cache.cc (right): http://codereview.chromium.org/140056/diff/1/4#newcode37 Line 37: #define NUMBER_OF_SUB_CACHES 4 Why not use constants for this? I would definitely have something like enum { kNumberOfSubCaches = 4, ... } or static const ints. http://codereview.chromium.org/140056/diff/1/4#newcode39 Line 39: // The number of generations for each sub cache. This should probably also be constants. http://codereview.chromium.org/140056/diff/1/4#newcode52 Line 52: static CompilationSubCache* entry[NUMBER_OF_SUB_CACHES] = entry seems like a weird name. How about subcaches? http://codereview.chromium.org/140056/diff/1/4#newcode73 Line 73: static const int kInitialCacheSize = 64; Maybe move this constant out to the other constant definitions? http://codereview.chromium.org/140056/diff/1/4#newcode87 Line 87: for (int generation = generations_ - 1; I would probably rewrite this to use |i| as the counter (like it's done in CompilationSubCache::Clear). http://codereview.chromium.org/140056/diff/1/4#newcode210 Line 210: Handle<CompilationCacheTable> table = GetTable(generation); Weird indentation. http://codereview.chromium.org/140056/diff/1/4#newcode249 Line 249: for (generation = 0; generation < generations(); generation++) { Weird indentation. http://codereview.chromium.org/140056/diff/1/2 File src/compilation-cache.h (right): http://codereview.chromium.org/140056/diff/1/2#newcode96 Line 96: // The compilation cache consists of several generational sub-caches which uses Could all this CompilationSubCache stuff be moved to compilation-cache.cc? It seems like it's an implementation detail for the CompilationCache. http://codereview.chromium.org/140056/diff/1/2#newcode103 Line 103: explicit CompilationSubCache(int generations): generations_(generations) { Space before :? http://codereview.chromium.org/140056/diff/1/2#newcode104 Line 104: // tables_ = new Object*[generations]; Remove. http://codereview.chromium.org/140056/diff/1/2#newcode107 Line 107: virtual ~CompilationSubCache() { Why is this virtual? Is it ever called and needed? http://codereview.chromium.org/140056 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
