Add version validation for LookupMap, also add unittest directly in LookupMap.

Add version validation for LookupMap, also add unittest directly in LookupMap.

R=sra@google.com

Committed: https://github.com/dart-lang/sdk/commit/fc582d7a3da8373a0b2d1e98e582afd2d00d6861

[Siggi Cherem (dart-lang), 2015-09-04 01:58:03]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2015-09-04 02:00:50]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2015-09-04 02:01:09]

Patchset #1 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2015-09-04 02:01:52]

sigmund@google.com changed reviewers:
+ sra@google.com

[Siggi Cherem (dart-lang), 2015-09-04 17:47:15]

Patchset #2 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2015-09-04 17:49:17]

just added a second patch to respect the abstraction between the compiler and
the backend.

This is the last change I was planning to do before publishing the package so
our users can start playing with it.

[Siggi Cherem (dart-lang), 2015-09-04 17:51:16]

+floitsch - What do you think of this approach to validate that we have the
package/library?

I think in general this could be another use case for exposing package-version
information as constants in programs.

[sra1, 2015-09-04 20:48:48]

lgtm

https://codereview.chromium.org/1312943007/diff/100001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1312943007/diff/100001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:2610: void onCodegenStart() =>
lookupMapAnalysis.onCodegenStart();
nit: I'd prefer these on-functions to be written with a body since we can add
new analyses here.

https://codereview.chromium.org/1312943007/diff/100001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/lookup_map_analysis.dart (right):

https://codereview.chromium.org/1312943007/diff/100001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/lookup_map_analysis.dart:151: // constant value
sohuld be available.
sohuld -> should

https://codereview.chromium.org/1312943007/diff/100001/pkg/lookup_map/lib/loo...
File pkg/lookup_map/lib/lookup_map.dart (right):

https://codereview.chromium.org/1312943007/diff/100001/pkg/lookup_map/lib/loo...
pkg/lookup_map/lib/lookup_map.dart:95: /// Internal constant that matches the
version in the pubspec. This is mainly
drop 'mainly' - does it really have another purpose?

[Siggi Cherem (dart-lang), 2015-09-04 20:57:00]

thanks!

https://codereview.chromium.org/1312943007/diff/100001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1312943007/diff/100001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:2610: void onCodegenStart() =>
lookupMapAnalysis.onCodegenStart();
On 2015/09/04 20:48:47, sra1 wrote:
> nit: I'd prefer these on-functions to be written with a body since we can add
> new analyses here.

Done, although I think of it as just being succinct, we can always make it a
block when needed :)

https://codereview.chromium.org/1312943007/diff/100001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/lookup_map_analysis.dart (right):

https://codereview.chromium.org/1312943007/diff/100001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/lookup_map_analysis.dart:151: // constant value
sohuld be available.
On 2015/09/04 20:48:47, sra1 wrote:
> sohuld -> should

Done.

https://codereview.chromium.org/1312943007/diff/100001/pkg/lookup_map/lib/loo...
File pkg/lookup_map/lib/lookup_map.dart (right):

https://codereview.chromium.org/1312943007/diff/100001/pkg/lookup_map/lib/loo...
pkg/lookup_map/lib/lookup_map.dart:95: /// Internal constant that matches the
version in the pubspec. This is mainly
On 2015/09/04 20:48:47, sra1 wrote:
> drop 'mainly' - does it really have another purpose?

Good point :). Done

[Siggi Cherem (dart-lang), 2015-09-04 20:59:56]

Committed patchset #3 (id:120001) manually as
fc582d7a3da8373a0b2d1e98e582afd2d00d6861 (presubmit successful).

[floitsch, 2015-09-07 10:48:01]

floitsch@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2015-09-07 10:48:02]

LGTM.

https://codereview.chromium.org/1312943007/diff/120001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/lookup_map_analysis.dart (right):

https://codereview.chromium.org/1312943007/diff/120001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/lookup_map_analysis.dart:136:
lookupMapVersionVariable = library.implementation.findLocal('_version');
I think I would have gone with an annotation, but this is pretty much the same.

[Bill Hesse, 2015-09-11 14:10:29]

whesse@google.com changed reviewers:
+ whesse@google.com

[Bill Hesse, 2015-09-11 14:10:29]

https://codereview.chromium.org/1312943007/diff/120001/pkg/lookup_map/test/ve...
File pkg/lookup_map/test/version_check_test.dart (right):

https://codereview.chromium.org/1312943007/diff/120001/pkg/lookup_map/test/ve...
pkg/lookup_map/test/version_check_test.dart:14: var pubspecPath =
Platform.script.resolve('../pubspec.yaml').path;
This is failing on windows because it uses .path instead of .toFilePath.

https://api.dartlang.org/1.12.1/dart-core/Uri/toFilePath.html

[Bill Hesse, 2015-09-11 14:17:56]

https://codereview.chromium.org/1312943007/diff/120001/pkg/lookup_map/test/ve...
File pkg/lookup_map/test/version_check_test.dart (right):

https://codereview.chromium.org/1312943007/diff/120001/pkg/lookup_map/test/ve...
pkg/lookup_map/test/version_check_test.dart:14: var pubspecPath =
Platform.script.resolve('../pubspec.yaml').path;
On 2015/09/11 14:10:29, Bill Hesse wrote:
> This is failing on windows because it uses .path instead of .toFilePath.
> 
> https://api.dartlang.org/1.12.1/dart-core/Uri/toFilePath.html

There is also new File.fromUri().

[Siggi Cherem (dart-lang), 2015-09-11 16:29:17]

https://codereview.chromium.org/1312943007/diff/120001/pkg/lookup_map/test/ve...
File pkg/lookup_map/test/version_check_test.dart (right):

https://codereview.chromium.org/1312943007/diff/120001/pkg/lookup_map/test/ve...
pkg/lookup_map/test/version_check_test.dart:14: var pubspecPath =
Platform.script.resolve('../pubspec.yaml').path;
On 2015/09/11 14:17:55, Bill Hesse wrote:
> On 2015/09/11 14:10:29, Bill Hesse wrote:
> > This is failing on windows because it uses .path instead of .toFilePath.
> > 
> > https://api.dartlang.org/1.12.1/dart-core/Uri/toFilePath.html
> 
> There is also new File.fromUri().

Oh - sorry about this, I didn't notice this failure. Thanks for sending the fix!