LGTM if comments are addressed.
https://codereview.chromium.org/868883002/diff/100001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/868883002/diff/100001/src/ast.h#newcode3171
src/ast.h:3171: explicit AstNodeFactory(Isolate* isolate,
AstValueFactory* ast_value_factory)
nit: The AstNodeFactory doesn't seem to use the Isolate, is there a
particular reason we need to pass it in?
https://codereview.chromium.org/868883002/diff/100001/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/868883002/diff/100001/src/compiler.cc#newcode765
src/compiler.cc:765: if (!AstNumbering::Renumber(info->isolate(),
info->zone(), info->function()))
nit: Can we put curly braces around body since conditional span more
than one line now?
https://codereview.chromium.org/868883002/diff/100001/src/compiler.cc#newcode1562
src/compiler.cc:1562: : name_(name), info_(info), zone_() {
nit: Can we drop explicit initializer call?
https://codereview.chromium.org/868883002/diff/100001/src/compiler.h
File src/compiler.h (right):
https://codereview.chromium.org/868883002/diff/100001/src/compiler.h#newcode532
src/compiler.h:532: : CompilationInfo(script, &zone_), zone_() {}
nit: It should be possible to just drop the initializer call to zone_
now since the implicit initializer is used anyways. Applies here and
three more times below.
https://codereview.chromium.org/868883002/diff/100001/src/compiler/graph-builder.h
File src/compiler/graph-builder.h (right):
https://codereview.chromium.org/868883002/diff/100001/src/compiler/graph-builder.h#newcode137
src/compiler/graph-builder.h:137: Isolate* isolate() const { return
isolate_; }
Can be dropped since it now just overrides GraphBuilder::isolate
anyways.
https://codereview.chromium.org/868883002/diff/100001/src/compiler/graph-builder.h#newcode157
src/compiler/graph-builder.h:157: Isolate* isolate_;
Can be dropped now since it shadows GraphBuilder::isolate_ now.
https://codereview.chromium.org/868883002/diff/100001/src/compiler/ia32/instruction-selector-ia32.cc
File src/compiler/ia32/instruction-selector-ia32.cc (right):
https://codereview.chromium.org/868883002/diff/100001/src/compiler/ia32/instruction-selector-ia32.cc#newcode34
src/compiler/ia32/instruction-selector-ia32.cc:34: return
!value.handle()->GetIsolate()->heap()->InNewSpace(
nit: Can we pull out the Isolate into a local variable here for
readability?
https://codereview.chromium.org/868883002/diff/100001/src/compiler/typer.h
File src/compiler/typer.h (right):
https://codereview.chromium.org/868883002/diff/100001/src/compiler/typer.h#newcode24
src/compiler/typer.h:24: explicit Typer(Isolate* isolate, Graph* graph,
MaybeHandle<Context> context);
nit: No longer needs to be marked "explicit".
https://codereview.chromium.org/868883002/diff/100001/src/compiler/zone-pool.cc
File src/compiler/zone-pool.cc (right):
https://codereview.chromium.org/868883002/diff/100001/src/compiler/zone-pool.cc#newcode68
src/compiler/zone-pool.cc:68: ZonePool::ZonePool(Isolate* isolate)
remark: Looks like we can also drop the Isolate parameter from the
ZonePool now, but that is for a separate CL, just wanted to point it
out.
https://codereview.chromium.org/868883002/diff/100001/src/full-codegen.h
File src/full-codegen.h (right):
https://codereview.chromium.org/868883002/diff/100001/src/full-codegen.h#newcode32
src/full-codegen.h:32: explicit BreakableStatementChecker(Isolate*
isolate, Zone* zone)
nit: No longer needs to be marked "explicit" now.
https://codereview.chromium.org/868883002/diff/100001/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/868883002/diff/100001/src/hydrogen.h#newcode2517
src/hydrogen.h:2517: Isolate* isolate() const { return
lookup_.isolate(); }
nit: Change looks unnecessary.
https://codereview.chromium.org/868883002/diff/100001/src/isolate.cc
File src/isolate.cc (right):
https://codereview.chromium.org/868883002/diff/100001/src/isolate.cc#newcode1647
src/isolate.cc:1647: runtime_zone_(),
nit: Explicit initializer call could be dropped.
https://codereview.chromium.org/868883002/diff/100001/src/json-parser.h
File src/json-parser.h (right):
https://codereview.chromium.org/868883002/diff/100001/src/json-parser.h#newcode35
src/json-parser.h:35: zone_(),
nit: Explicit initializer call could be dropped.
https://codereview.chromium.org/868883002/diff/100001/src/lithium-allocator.cc
File src/lithium-allocator.cc (right):
https://codereview.chromium.org/868883002/diff/100001/src/lithium-allocator.cc#newcode514
src/lithium-allocator.cc:514: : zone_(),
nit: Explicit initializer call could be dropped.
https://codereview.chromium.org/868883002/diff/100001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/868883002/diff/100001/src/preparser.h#newcode597
src/preparser.h:597: Isolate* isolate() const { return isolate_; }
nit: Could we move this up to around line 300 near the other getters for
readability?
https://codereview.chromium.org/868883002/diff/100001/src/prettyprinter.h
File src/prettyprinter.h (right):
https://codereview.chromium.org/868883002/diff/100001/src/prettyprinter.h#newcode16
src/prettyprinter.h:16: explicit CallPrinter(Isolate* isolate, Zone*
zone);
nit: No longer needs to be marked "explicit".
https://codereview.chromium.org/868883002/diff/100001/src/prettyprinter.h#newcode55
src/prettyprinter.h:55: explicit PrettyPrinter(Isolate* isolate, Zone*
zone);
nit: No longer needs to be marked "explicit".
https://codereview.chromium.org/868883002/diff/100001/src/prettyprinter.h#newcode101
src/prettyprinter.h:101: explicit AstPrinter(Isolate* isolate, Zone*
zone);
nit: No longer needs to be marked "explicit".
https://codereview.chromium.org/868883002/diff/100001/src/regexp-macro-assembler-tracer.h
File src/regexp-macro-assembler-tracer.h (right):
https://codereview.chromium.org/868883002/diff/100001/src/regexp-macro-assembler-tracer.h#newcode14
src/regexp-macro-assembler-tracer.h:14: explicit
RegExpMacroAssemblerTracer(Isolate* isolate,
nit: No longer needs to be marked "explicit".
https://codereview.chromium.org/868883002/diff/100001/src/regexp-macro-assembler.h
File src/regexp-macro-assembler.h (right):
https://codereview.chromium.org/868883002/diff/100001/src/regexp-macro-assembler.h#newcode46
src/regexp-macro-assembler.h:46: explicit RegExpMacroAssembler(Isolate*
isolate, Zone* zone);
nit: No longer needs to be marked "explicit".
https://codereview.chromium.org/868883002/diff/100001/src/regexp-macro-assembler.h#newcode190
src/regexp-macro-assembler.h:190: explicit
NativeRegExpMacroAssembler(Isolate* isolate, Zone* zone);
nit: No longer needs to be marked "explicit".
https://codereview.chromium.org/868883002/diff/100001/test/cctest/cctest.h
File test/cctest/cctest.h (right):
https://codereview.chromium.org/868883002/diff/100001/test/cctest/cctest.h#newcode580
test/cctest/cctest.h:580: HandleAndZoneScope() : main_zone_() {}
nit: Explicit initializer call can be dropped.
https://codereview.chromium.org/868883002/diff/100001/test/cctest/compiler/graph-builder-tester.h
File test/cctest/compiler/graph-builder-tester.h (right):
https://codereview.chromium.org/868883002/diff/100001/test/cctest/compiler/graph-builder-tester.h#newcode25
test/cctest/compiler/graph-builder-tester.h:25: explicit
DirectGraphBuilder(Isolate* isolate, Graph* graph)
nit: No longer needs to be marked "explicit".
https://codereview.chromium.org/868883002/diff/100001/test/cctest/compiler/test-js-constant-cache.cc
File test/cctest/compiler/test-js-constant-cache.cc (right):
https://codereview.chromium.org/868883002/diff/100001/test/cctest/compiler/test-js-constant-cache.cc#newcode20
test/cctest/compiler/test-js-constant-cache.cc:20: explicit
JSCacheTesterHelper(Isolate* isolate, Zone* zone)
nit: No longer needs to be marked "explicit".
https://codereview.chromium.org/868883002/diff/100001/test/unittests/test-utils.h
File test/unittests/test-utils.h (right):
https://codereview.chromium.org/868883002/diff/100001/test/unittests/test-utils.h#newcode95
test/unittests/test-utils.h:95: TestWithZone() : zone_() {}
nit: Explicit initializer call shouldn't be needed anymore.
https://codereview.chromium.org/868883002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
