Issue 11421215: Support symbolic links and package imports in gen_snapshot.

Issue 11421215: Support symbolic links and package imports in gen_snapshot. (Closed)

Created:
8 years ago by podivilov

Modified:
8 years ago

Reviewers:
siva, Ivan Posva

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Support symbolic links and package imports in gen_snapshot. R=asiva@google.com,iposva@google.com Committed: https://code.google.com/p/dart/source/detail?r=15702

Patch Set 2 : . #

Created: 8 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+253 lines, -51 lines)			Patch
M	runtime/bin/dartutils.h	View		1 chunk	+13 lines, -0 lines	0 comments	Download
M	runtime/bin/dartutils.cc	View		4 chunks	+26 lines, -21 lines	0 comments	Download
M	runtime/bin/gen_snapshot.cc	View	1	5 chunks	+214 lines, -30 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

siva

LGTM with two comments. https://codereview.chromium.org/11421215/diff/1/runtime/bin/gen_snapshot.cc File runtime/bin/gen_snapshot.cc (right): https://codereview.chromium.org/11421215/diff/1/runtime/bin/gen_snapshot.cc#newcode401 runtime/bin/gen_snapshot.cc:401: static void SetupForGenericSnapshotCreation(bool script_snapshot) { ...

8 years ago (2012-12-04 20:00:02 UTC) #2

podivilov

8 years ago (2012-12-04 20:20:01 UTC) #3

https://codereview.chromium.org/11421215/diff/1/runtime/bin/gen_snapshot.cc
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/11421215/diff/1/runtime/bin/gen_snapshot.cc#n...
runtime/bin/gen_snapshot.cc:401: static void
SetupForGenericSnapshotCreation(bool script_snapshot) {
On 2012/12/04 20:00:02, siva wrote:
> It would probably be more readable if you split this into
> static void SetupForURLResolution() {
>   // Setup builtin, uri and utf.
> }
> static void SetupForGenericSnapshotCreation() {
>   SetupForURLResolution();
>   // Setup json, cryptom, IO
> }
> 
> That way we don't have this boolean flag as a parameter which makes it
confusing
> and also would take care of the issue I raised below (see note for the full
> snapshot case).

Done.

https://codereview.chromium.org/11421215/diff/1/runtime/bin/gen_snapshot.cc#n...
runtime/bin/gen_snapshot.cc:572: SetupForGenericSnapshotCreation(false);
On 2012/12/04 20:00:02, siva wrote:
> For a full standalone snapshot we need to load all the libraries.

Done.

Expand Messages | Collapse Messages