"Implement" #resource tag and various restrictions on library definitions.

"Implement" #resource tag and various restrictions on library definitions.

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

[ngeoffray, 2012-03-23 07:54:30]

LGTM

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
File dart/frog/leg/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:31: compiler.reportError(tag, 'out of
order');
That's news to me. Not sure I like it.

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:129: if (imported.tags.isEmpty()) {
Please add a comment that since we're checking the order of the tags, if the
tags are not empty, we are sure there is a library tag.

You could also add a helper method in LibraryElement: hasLibraryTag.

[Lasse Reichstein Nielsen, 2012-03-23 08:10:58]

LGTM

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
File dart/frog/leg/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:6: static final int LIBRARY = 1;
Add comment saying that these are numbered in the order they must occur in the
source file.

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:8: static final int SOURCE = 3;
The spec uses the word "include" about the production. Perhaps change SOURCE to
INCLUDE.

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:27: int tagState = 0;
Name is undescriptive. It is being used to remember the currently expected tag
type, so perhaps call it something like "expectedTag".

Could start at LIBRARY?

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:29: int checkTag(int currentState,
ScriptTag tag) {
Add comment explaining that checkTag returns the [currentState] argument on
success, and otherwise the current expected tag (i.e., on error, don't change
the expectation).

Alternatively, you could return the [currentState] in both cases, so a sequence
of
#source("aaa")
#library("bbb")
#import("bbb")
would only warn about the library, and not both library and import. The real
warning could even be saying that #source and #import occur out of order (the
bug here is likely to be the source, not the rest).

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:30: if (tagState > currentState) {
Rename currentState to, e.g., newState. It's too easy to confuse it with the
current tagState.

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:53: tagState = checkTag(LIBRARY, tag) +
1;
This is too cute.

If the check fails, you increment the expectation, so

#import()
#library()
#import()

will give an error on the second import.
You might want 
  tagState = Math.min(checkTag(LIBRARY, tag), IMPORT));

[ahe, 2012-03-23 08:44:35]

Hi Lasse and Nicolas,

Thank you for your suggestions. I have replied to some of your comments below.

I plan to add a class like this:

/** <<explain what the states do>> */ 
class TagState {
  static final int NO_TAG = 0;
  static final int LIBRARY = 1;
  ...
}

Cheers,
Peter

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
File dart/frog/leg/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:8: static final int SOURCE = 3;
On 2012/03/23 08:10:59, Lasse Reichstein Nielsen wrote:
> The spec uses the word "include" about the production. Perhaps change SOURCE
to
> INCLUDE.

I prefer the word "source" because I find it confusing to have both "include"
and "import". I think this was why the keywords were originally chosen and I
consider it a problem that the spec attempts to rename the concept
inconsistently. However, "source" is still terminology from the specification
simply because that is the keyword we're using.

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:27: int tagState = 0;
On 2012/03/23 08:10:59, Lasse Reichstein Nielsen wrote:
> Name is undescriptive. It is being used to remember the currently expected tag
> type, so perhaps call it something like "expectedTag".
> 
> Could start at LIBRARY?

There is something about starting a zero that helps me understand the code
better. I think the code will make more sense if I create a class named TagState
and elaborate a little in its documentation comment.

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:31: compiler.reportError(tag, 'out of
order');
On 2012/03/23 07:54:30, ngeoffray wrote:
> That's news to me. Not sure I like it.

This is the same problem we have with final, static, and abstract. We wrote a
simple grammar in ANTLR and kinda forgot that we specified an explicit order.

Long term, I think we need to relax these restrictions, but right now, I think
it is acceptable to follow the strict interpretation of the grammar. We have far
more important things to trouble the VM team about.

[ahe, 2012-03-23 17:15:22]

PTAL

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
File dart/frog/leg/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:30: if (tagState > currentState) {
On 2012/03/23 08:10:59, Lasse Reichstein Nielsen wrote:
> Rename currentState to, e.g., newState. It's too easy to confuse it with the
> current tagState.

I agree. I can't come up with anything better so I have used "value".

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:53: tagState = checkTag(LIBRARY, tag) +
1;
On 2012/03/23 08:10:59, Lasse Reichstein Nielsen wrote:
> This is too cute.
> 
> If the check fails, you increment the expectation, so
> 
> #import()
> #library()
> #import()
> 
> will give an error on the second import.
> You might want 
>   tagState = Math.min(checkTag(LIBRARY, tag), IMPORT));
> 

Done.

https://chromiumcodereview.appspot.com/9838038/diff/1/dart/frog/leg/scanner/s...
dart/frog/leg/scanner/scanner_task.dart:129: if (imported.tags.isEmpty()) {
On 2012/03/23 07:54:30, ngeoffray wrote:
> Please add a comment that since we're checking the order of the tags, if the
> tags are not empty, we are sure there is a library tag.
> 
> You could also add a helper method in LibraryElement: hasLibraryTag.

This is actually not correct. I forgot to add the else branch.

[ngeoffray, 2012-03-23 17:22:51]

Still LGTM

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

https://chromiumcodereview.appspot.com/9838038/diff/3002/dart/frog/leg/elemen...
dart/frog/leg/elements/elements.dart:317: bool hasLibraryName() =>
(!tags.isEmpty()) && tags.reverse().head.isLibrary();
The 'reverse' call looks sooooooo expensive for so little.

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

https://chromiumcodereview.appspot.com/9838038/diff/3002/dart/frog/leg/scanne...
dart/frog/leg/scanner/scanner_task.dart:199: const <int>[NO_TAG_SEEN, IMPORT,
IMPORT, SOURCE, RESOURCE];
Please add a comment about duplicating IMPORT but not having LIBRARY.

[ahe, 2012-03-24 08:02:03]

Thank you.

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

https://chromiumcodereview.appspot.com/9838038/diff/3002/dart/frog/leg/elemen...
dart/frog/leg/elements/elements.dart:317: bool hasLibraryName() =>
(!tags.isEmpty()) && tags.reverse().head.isLibrary();
On 2012/03/23 17:22:52, ngeoffray wrote:
> The 'reverse' call looks sooooooo expensive for so little.

Especially when the library tag is recorded already in the field named
[libraryTag]. Doh!

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

https://chromiumcodereview.appspot.com/9838038/diff/3002/dart/frog/leg/scanne...
dart/frog/leg/scanner/scanner_task.dart:199: const <int>[NO_TAG_SEEN, IMPORT,
IMPORT, SOURCE, RESOURCE];
On 2012/03/23 17:22:52, ngeoffray wrote:
> Please add a comment about duplicating IMPORT but not having LIBRARY.

Done.

[ahe, 2012-03-24 08:40:14]

I had to make a few tweaks after rebasing/updating. Unfortunately, I forgot to
upload a clean version before I started making changes, so I have added comments
to help you identify the stuff I didn't touch in patch set #3.

https://chromiumcodereview.appspot.com/9838038/diff/1009/dart/frog/leg/tree/n...
File dart/frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9838038/diff/1009/dart/frog/leg/tree/n...
dart/frog/leg/tree/nodes.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
Changes in this file only due to updating.

https://chromiumcodereview.appspot.com/9838038/diff/1009/dart/tests/co19/co19...
File dart/tests/co19/co19-leg.status (right):

https://chromiumcodereview.appspot.com/9838038/diff/1009/dart/tests/co19/co19...
dart/tests/co19/co19-leg.status:1: # Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
Changes in this file only due to updating.

https://chromiumcodereview.appspot.com/9838038/diff/1009/dart/tests/language/...
File dart/tests/language/language-leg.status (right):

https://chromiumcodereview.appspot.com/9838038/diff/1009/dart/tests/language/...
dart/tests/language/language-leg.status:1: # Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
Changes in this file only due to updating.