Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2024473002:
WIP: prototype resolver script (Closed)
|
Created:
4 years, 6 months ago by Harry Terkelsen Modified:
4 years, 6 months ago Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
Descriptionadd resolver dart2js script
can resolve helloworld
BUG=
Committed: https://github.com/dart-lang/sdk/commit/6b721d51455881d43a62a8433feab779ff356188
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Messages
Total messages: 14 (3 generated)
het@google.com changed reviewers: + johnniwinther@google.com, sigmund@google.com
please take a look at this prototype and let me know if you think this is the right direction thanks!
beautiful! :) https://codereview.chromium.org/2024473002/diff/1/pkg/compiler/bin/resolver.dart File pkg/compiler/bin/resolver.dart (right): https://codereview.chromium.org/2024473002/diff/1/pkg/compiler/bin/resolver.d... pkg/compiler/bin/resolver.dart:56: .where((lib) => inputs.contains(lib.canonicalUri)) consider making inputs a set instead of a list to make these checks faster.
https://codereview.chromium.org/2024473002/diff/1/pkg/compiler/bin/resolver.dart File pkg/compiler/bin/resolver.dart (right): https://codereview.chromium.org/2024473002/diff/1/pkg/compiler/bin/resolver.d... pkg/compiler/bin/resolver.dart:56: .where((lib) => inputs.contains(lib.canonicalUri)) On 2016/05/27 23:38:55, Siggi Cherem (dart-lang) wrote: > consider making inputs a set instead of a list to make these checks faster. i realized there is a better way, just ask the libraryLoader for the element associated with each input library
https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... File pkg/compiler/bin/resolver.dart (right): https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:24: args['deps'].map((uri) => currentDirectory.resolve(uri)).toList(); Use `currentDirectory.resolve(nativeToUriPath(uri))` to handle both 'foo/bar' and 'foo\bar' on Windows. https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:25: var root = uriPathToNative("../../../sdk"); This should be [nativeToUriPath], otherwise on Windows you feed '..\..\..\sdk/root' to `Platform.script.resolve` below. https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:39: var inputs = args.rest.map((uri) => currentDirectory.resolve(uri)).toList(); Use `nativeToUriPath` here as well.
Thanks, Johnni! https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... File pkg/compiler/bin/resolver.dart (right): https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:24: args['deps'].map((uri) => currentDirectory.resolve(uri)).toList(); On 2016/05/30 11:09:54, Johnni Winther wrote: > Use `currentDirectory.resolve(nativeToUriPath(uri))` to handle both 'foo/bar' > and 'foo\bar' on Windows. Done. https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:25: var root = uriPathToNative("../../../sdk"); On 2016/05/30 11:09:54, Johnni Winther wrote: > This should be [nativeToUriPath], otherwise on Windows you feed > '..\..\..\sdk/root' to `Platform.script.resolve` below. Done. https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:39: var inputs = args.rest.map((uri) => currentDirectory.resolve(uri)).toList(); On 2016/05/30 11:09:54, Johnni Winther wrote: > Use `nativeToUriPath` here as well. Done.
https://codereview.chromium.org/2024473002/diff/40001/pkg/compiler/bin/resolv... File pkg/compiler/bin/resolver.dart (right): https://codereview.chromium.org/2024473002/diff/40001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:26: var root = nativeToUriPath("../../../sdk"); BTW - we'll need a flag for this too. That will let us adapt to build systems where the sdk is not in the same relative location as this binary file. https://codereview.chromium.org/2024473002/diff/40001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:65: var outFile = args['out'] ?? 'out.json'; minor nit: since we plan to change the data to be some binary format soon, we were thinking of using .data instead of .json.
https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... File pkg/compiler/bin/resolver.dart (right): https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:25: var root = uriPathToNative("../../../sdk"); On 2016/05/31 16:28:27, Harry Terkelsen wrote: > On 2016/05/30 11:09:54, Johnni Winther wrote: > > This should be [nativeToUriPath], otherwise on Windows you feed > > '..\..\..\sdk/root' to `Platform.script.resolve` below. > > Done. Actually here it was already in URL form, seems like we don't need to convert in either direction?
https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... File pkg/compiler/bin/resolver.dart (right): https://codereview.chromium.org/2024473002/diff/20001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:25: var root = uriPathToNative("../../../sdk"); On 2016/06/02 00:20:36, Siggi Cherem (dart-lang) wrote: > On 2016/05/31 16:28:27, Harry Terkelsen wrote: > > On 2016/05/30 11:09:54, Johnni Winther wrote: > > > This should be [nativeToUriPath], otherwise on Windows you feed > > > '..\..\..\sdk/root' to `Platform.script.resolve` below. > > > > Done. > > Actually here it was already in URL form, seems like we don't need to convert in > either direction? Correct
https://codereview.chromium.org/2024473002/diff/40001/pkg/compiler/bin/resolv... File pkg/compiler/bin/resolver.dart (right): https://codereview.chromium.org/2024473002/diff/40001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:26: var root = nativeToUriPath("../../../sdk"); On 2016/06/01 23:58:57, Siggi Cherem (dart-lang) wrote: > BTW - we'll need a flag for this too. > > That will let us adapt to build systems where the sdk is not in the same > relative location as this binary file. Done. https://codereview.chromium.org/2024473002/diff/40001/pkg/compiler/bin/resolv... pkg/compiler/bin/resolver.dart:65: var outFile = args['out'] ?? 'out.json'; On 2016/06/01 23:58:57, Siggi Cherem (dart-lang) wrote: > minor nit: since we plan to change the data to be some binary format soon, we > were thinking of using .data instead of .json. Done.
Description was changed from ========== WIP: prototype resolver script can resolve helloworld BUG= ========== to ========== add resolver dart2js script can resolve helloworld BUG= ==========
Description was changed from ========== add resolver dart2js script can resolve helloworld BUG= ========== to ========== add resolver dart2js script can resolve helloworld BUG= Committed: https://github.com/dart-lang/sdk/commit/6b721d51455881d43a62a8433feab779ff356188 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 6b721d51455881d43a62a8433feab779ff356188 (presubmit successful).
Message was sent while issue was closed.
lgtm |
