Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(525)

Issue 12047096: Get rid of RootSource. (Closed)

Created:
7 years, 11 months ago by Bob Nystrom
Modified:
7 years, 11 months ago
Reviewers:
Jennifer Messerly
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Get rid of RootSource. This was a sort of hackish source only used internally during version solving. But it causes a couple of subtle bugs because it never gets registered with the SourceRegistry. That in turn makes toString() fail on any PackageId with that source since it relies on the Source being registered. The simplest fix is to just remove the hacked source and have a null source in PackageId/Ref mean "root" and handle it specially. Committed: https://code.google.com/p/dart/source/detail?r=17646

Patch Set 1 #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -55 lines) Patch
M utils/pub/entrypoint.dart View 3 chunks +2 lines, -3 lines 0 comments Download
M utils/pub/package.dart View 5 chunks +24 lines, -5 lines 6 comments Download
D utils/pub/root_source.dart View 1 chunk +0 lines, -30 lines 0 comments Download
M utils/pub/sdk.dart View 1 chunk +1 line, -1 line 0 comments Download
M utils/pub/version_solver.dart View 5 chunks +7 lines, -11 lines 0 comments Download
M utils/tests/pub/version_solver_test.dart View 4 chunks +1 line, -5 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Bob Nystrom
Hey, Nathan is on vacation. Want to be a pub reviewer?
7 years, 11 months ago (2013-01-24 22:18:40 UTC) #1
Jennifer Messerly
lgtm https://codereview.chromium.org/12047096/diff/1/utils/pub/package.dart File utils/pub/package.dart (right): https://codereview.chromium.org/12047096/diff/1/utils/pub/package.dart#newcode86 utils/pub/package.dart:86: /// this is a root package ID, this ...
7 years, 11 months ago (2013-01-24 23:24:37 UTC) #2
Bob Nystrom
7 years, 11 months ago (2013-01-25 18:22:50 UTC) #3
Message was sent while issue was closed.
Thanks!

https://codereview.chromium.org/12047096/diff/1/utils/pub/package.dart
File utils/pub/package.dart (right):

https://codereview.chromium.org/12047096/diff/1/utils/pub/package.dart#newcode86
utils/pub/package.dart:86: /// this is a root package ID, this will be `null`.
On 2013/01/24 23:24:37, John Messerly wrote:
> fwiw, I think Kathy suggested not using code font for `null`, as a general
> desire to avoid too much code font. Up to you though. I might be
misremembering
> this.

I think you're remembering right, though I personally do prefer using code font
for literals. I like a little visual style variation and I think code font can
be important to disambiguate use-mention in cases where a literal is a valid
word like "this".

https://codereview.chromium.org/12047096/diff/1/utils/pub/package.dart#newcod...
utils/pub/package.dart:104: (source != null ? source.name.hashCode : 0) ^
On 2013/01/24 23:24:37, John Messerly wrote:
> Another idea here: you could make Source.hashCode return its name.hashCode:
> 
>     class Source { ...; int get hashCode => name.hashCode; }
> 
> Then change this one to be:
> 
>     int get hashCode => name.hashCode ^ source.hashCode ^ version.hashCode
> 
> 
> That might be cleaner? Basically, exploit the fact that "null.hashCode" works
in
> Dart :)

Great idea! Done.

https://codereview.chromium.org/12047096/diff/1/utils/pub/package.dart#newcod...
utils/pub/package.dart:113: other.source == source &&
On 2013/01/24 23:24:37, John Messerly wrote:
> just checking, source already implements reasonable equality operator? Or do
we
> know that identity semantics work for it?

I'm relying on identity here. Sources are effectively singletons and weird
things would happen if you had more than one source of the same type.

Powered by Google App Engine
This is Rietveld 408576698