Source in most stuff.

Source in most stuff.

Committed: https://code.google.com/p/dart/source/detail?r=4270

[kasperl, 2012-02-15 09:22:29]

LGTM!

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/elemen...
File dart/frog/leg/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/elemen...
dart/frog/leg/elements/elements.dart:326: Type type =
compiler.types.lookup(name);
Would it be cleaner to have a way of querying if the types contains a certain
name and deal with that first? I'm always a bit worried when I see lookup
functions that happily return null.

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/scanne...
File dart/frog/leg/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/scanne...
dart/frog/leg/scanner/scanner_task.dart:20: Uri cwd = new Uri(scheme: 'file',
path: compiler.currentDirectory);
The named arguments are cool. Would it make sense to split this method into two
parts? One that returns the sources and imports and checks that the tags are
valid -- and one that does the readScript, importLibrary calls? You almost have
that split now.

[ahe, 2012-02-15 09:46:55]

Hi Kasper,

Thank you for your comments.

Cheers,
Peter

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/elemen...
File dart/frog/leg/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/elemen...
dart/frog/leg/elements/elements.dart:326: Type type =
compiler.types.lookup(name);
On 2012/02/15 09:22:30, kasperl wrote:
> Would it be cleaner to have a way of querying if the types contains a certain
> name and deal with that first? I'm always a bit worried when I see lookup
> functions that happily return null.

Interesting difference of opinion here: I really prefer avoiding looking up
something twice. Or is it the name of the method that you don't like?

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/scanne...
File dart/frog/leg/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/scanne...
dart/frog/leg/scanner/scanner_task.dart:20: Uri cwd = new Uri(scheme: 'file',
path: compiler.currentDirectory);
On 2012/02/15 09:22:30, kasperl wrote:
> The named arguments are cool. Would it make sense to split this method into
two
> parts? One that returns the sources and imports and checks that the tags are
> valid -- and one that does the readScript, importLibrary calls? You almost
have
> that split now.

I need to process source directives before I start importing. So it would look
something like this:

Link<ScriptTag> tags = checkTags();
for (ScriptTag tag in tags) {
  if (tag.isSource()) sourceScript(tag);
}
for (ScriptTag tag in tags) {
  if (tag.isImport()) importLibrary(tag);
}

Perhaps I can make this a little cleaner in a future CL.

[ngeoffray, 2012-02-15 09:53:03]

LGTM!

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/scanne...
File dart/frog/leg/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/scanne...
dart/frog/leg/scanner/scanner_task.dart:42: for (ScriptTag tag in
imports.toLink()) {
Maybe add a comment on why you need to import last.

[kasperl, 2012-02-15 10:01:29]

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/elemen...
File dart/frog/leg/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/elemen...
dart/frog/leg/elements/elements.dart:326: Type type =
compiler.types.lookup(name);
On 2012/02/15 09:46:55, ahe wrote:
> On 2012/02/15 09:22:30, kasperl wrote:
> > Would it be cleaner to have a way of querying if the types contains a
certain
> > name and deal with that first? I'm always a bit worried when I see lookup
> > functions that happily return null.
> 
> Interesting difference of opinion here: I really prefer avoiding looking up
> something twice. Or is it the name of the method that you don't like?

Yeah, I think it would be neater with something like 

compiler.types.lookupIfAbsent(
  name, 
  () => compiler.types.dynamicType);

or maybe just move the logic into compiler.types.lookup. Usually I prefer having
something like lookup throw an error if it cannot find the thing you're looking
for because it is so easy to write code under that assumption.

[ahe, 2012-02-15 15:08:41]

Hi Nicolas and Kasper,

Thank you for your comments.

Cheers,
Peter

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/elemen...
File dart/frog/leg/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/elemen...
dart/frog/leg/elements/elements.dart:326: Type type =
compiler.types.lookup(name);
On 2012/02/15 10:01:29, kasperl wrote:
> On 2012/02/15 09:46:55, ahe wrote:
> > On 2012/02/15 09:22:30, kasperl wrote:
> > > Would it be cleaner to have a way of querying if the types contains a
> certain
> > > name and deal with that first? I'm always a bit worried when I see lookup
> > > functions that happily return null.
> > 
> > Interesting difference of opinion here: I really prefer avoiding looking up
> > something twice. Or is it the name of the method that you don't like?
> 
> Yeah, I think it would be neater with something like 
> 
> compiler.types.lookupIfAbsent(
>   name, 
>   () => compiler.types.dynamicType);
> 
> or maybe just move the logic into compiler.types.lookup. Usually I prefer
having
> something like lookup throw an error if it cannot find the thing you're
looking
> for because it is so easy to write code under that assumption.

Actually, this entire method is somewhat broken. I plan to overhaul this a some
point, but I'm not sure what to do. Karl and I had a conversation about this,
but we didn't come up with anything yet.

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/scanne...
File dart/frog/leg/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/9359039/diff/3001/dart/frog/leg/scanne...
dart/frog/leg/scanner/scanner_task.dart:42: for (ScriptTag tag in
imports.toLink()) {
On 2012/02/15 09:53:03, ngeoffray wrote:
> Maybe add a comment on why you need to import last.

Done.