Verify that cache unlinked and linked bundles are majorVersion compatible.

Verify that cache unlinked and linked bundles are majorVersion compatible.


R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[Brian Wilkerson, 2016-09-08 16:42:28]

lgtm

https://codereview.chromium.org/2328613002/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/pub_summary.dart (right):

https://codereview.chromium.org/2328613002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/pub_summary.dart:87: int majorVersion =
PackageBundleAssembler.currentMajorVersion;
Should this be final?

[scheglov, 2016-09-08 16:44:24]

https://codereview.chromium.org/2328613002/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/pub_summary.dart (right):

https://codereview.chromium.org/2328613002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/pub_summary.dart:87: int majorVersion =
PackageBundleAssembler.currentMajorVersion;
On 2016/09/08 16:42:28, Brian Wilkerson wrote:
> Should this be final?

No.
The only reason it was added is to be able to change it in tests.
In theory it could be final, and passed in the constructor.
But that seems like an overhead for the test-only feature.

[Paul Berry, 2016-09-08 16:45:17]

lgtm

https://codereview.chromium.org/2328613002/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/pub_summary.dart (right):

https://codereview.chromium.org/2328613002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/pub_summary.dart:87: int majorVersion =
PackageBundleAssembler.currentMajorVersion;
On 2016/09/08 16:42:28, Brian Wilkerson wrote:
> Should this be final?

I was just about to make a similar comment.  I'd prefer if we made this final
and initialized it in the constructor using a named argument whose default value
is PackageBundleAssembler.currentMajorVersion.  That way we can still create a
PubSummaryManager during tests that uses a different major version, but there's
no worry about this field getting modified after the class is created and
causing weird behaviors.

[scheglov, 2016-09-08 17:02:38]

https://codereview.chromium.org/2328613002/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/pub_summary.dart (right):

https://codereview.chromium.org/2328613002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/pub_summary.dart:87: int majorVersion =
PackageBundleAssembler.currentMajorVersion;
On 2016/09/08 16:45:16, Paul Berry wrote:
> On 2016/09/08 16:42:28, Brian Wilkerson wrote:
> > Should this be final?
> 
> I was just about to make a similar comment.  I'd prefer if we made this final
> and initialized it in the constructor using a named argument whose default
value
> is PackageBundleAssembler.currentMajorVersion.  That way we can still create a
> PubSummaryManager during tests that uses a different major version, but
there's
> no worry about this field getting modified after the class is created and
> causing weird behaviors.

OK, it seems that we have a consensus here :-)
Done.

[scheglov, 2016-09-08 17:02:56]

Description was changed from

==========
Verify that cache unlinked and linked bundles are majorVersion compatible.


R=brianwilkerson@google.com, paulberry@google.com
BUG=
==========

to

==========
Verify that cache unlinked and linked bundles are majorVersion compatible.


R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[scheglov, 2016-09-08 17:02:57]

Committed patchset #2 (id:20001) manually as
ba46c2d8e973033e8273a963f052db8a52e59429 (presubmit successful).