Almost looks good. Just a few naming and stylistic nitpicks.
Thanks, Vitaly http://codereview.chromium.org/2657004/diff/21001/22003 File src/assembler.cc (right): http://codereview.chromium.org/2657004/diff/21001/22003#newcode593 src/assembler.cc:593: cache_array_address()); nit: 4-space indent (relative to previous line) for line continuations. http://codereview.chromium.org/2657004/diff/21001/22006 File src/heap-inl.h (right): http://codereview.chromium.org/2657004/diff/21001/22006#newcode512 src/heap-inl.h:512: nit: Keep two blank lines between top-level constructs. (Also below.) http://codereview.chromium.org/2657004/diff/21001/22005 File src/heap.h (right): http://codereview.chromium.org/2657004/diff/21001/22005#newcode1811 src/heap.h:1811: class TranscendentalCacheTypes { We don't usually create namespace-like classes to hold enums. Instead enums are either made global or put into the main class that uses them. Move the enum to be right before TrancendentalCache::Get(Type, double) function. http://codereview.chromium.org/2657004/diff/21001/22005#newcode1815 src/heap.h:1815: nit: Keep two blank lines between top-level constructs. http://codereview.chromium.org/2657004/diff/21001/22005#newcode1818 src/heap.h:1818: class TranscendentalCacheData: public TranscendentalCacheTypes { Since this is a "frontend" to caching functionality let's name it "TranscendtalCache". http://codereview.chromium.org/2657004/diff/21001/22005#newcode1834 src/heap.h:1834: friend class TranscendentalCache; Is this required? http://codereview.chromium.org/2657004/diff/21001/22005#newcode1842 src/heap.h:1842: TranscendentalCacheData() { This should be the first thing in the private section. http://codereview.chromium.org/2657004/diff/21001/22005#newcode1849 src/heap.h:1849: class TranscendentalCache: public TranscendentalCacheTypes { "TranscendentalCacheData". You could also make this an inner class of the cache. http://codereview.chromium.org/2657004/diff/21001/22005#newcode1920 src/heap.h:1920: }; Add DISALLOW_COPY_AND_ASSIGN. http://codereview.chromium.org/2657004/diff/21001/22001 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/2657004/diff/21001/22001#newcode10308 src/ia32/codegen-ia32.cc:10308: Isolate::Current()->transcendental_cache_data()->caches_[0]))); nit: 4-space indent (relative to previous line) for line continuations. (Also below.) http://codereview.chromium.org/2657004/diff/21001/22007 File src/isolate.h (right): http://codereview.chromium.org/2657004/diff/21001/22007#newcode89 src/isolate.h:89: TranscendentalCacheData* transcendental_cache_data() const { "trancendental_cache". http://codereview.chromium.org/2657004/diff/21001/22009 File src/runtime.cc (right): http://codereview.chromium.org/2657004/diff/21001/22009#newcode5906 src/runtime.cc:5906: Get(TranscendentalCache::ACOS, x); nit: Indent. (Also below.) http://codereview.chromium.org/2657004/diff/21001/22002 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/2657004/diff/21001/22002#newcode8191 src/x64/codegen-x64.cc:8191: transcendental_cache_data()->caches_[0]))); nit: Indent. http://codereview.chromium.org/2657004/diff/21001/22010 File test/cctest/test-serialize.cc (right): http://codereview.chromium.org/2657004/diff/21001/22010#newcode108 test/cctest/test-serialize.cc:108: v8::V8::Initialize(); It should work with just Heap::Setup(false) call since now we pre-initialize the default isolate. http://codereview.chromium.org/2657004/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
