Looking good, just one suggestion.
https://codereview.chromium.org/1304923004/diff/100001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1304923004/diff/100001/src/parser.cc#newcode4241
src/parser.cc:4241: factory()->set_zone(outer_zone);
Suggestion as discsussed offline: This explicit resetting to the
previous zone could become brittle should the thing it is scoping become
more complex. Also exposing AstFactory::set_zone can be abused. One
counter proposal would be to slightly change the API. I was thinking
about something along the lines of:
{
Zone temp_zone;
bool can_use_temp_zone = ...;
AstNodeFactory::ZoneScope scope(factory(), &temp_zone,
can_use_temp_zone);
...
}
This way we gain two things:
1) There is no need to expose set_zone() to the general public, the new
scope is the only one being able to change the zone.
2) Even if the thing being covered by the scope gets more complex and
early bailout return statements are added, the resetting to the original
zone and the destruction of the body still happens.
WDYT?
https://codereview.chromium.org/1304923004/
--
--
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.
