Implement argument definition test in the vm.

Implement argument definition test in the vm.
Add tests.
Various cleanups.
Committed: https://code.google.com/p/dart/source/detail?r=11664

[srdjan, 2012-08-30 22:15:59]

LGTM

https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/ast.h#newco...
runtime/vm/ast.h:250: explicit ArgumentDefinitionTestNode(intptr_t token_pos,
remove explicit

https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_ia32.cc:964: __ movl(Address(EBP, slot *
kWordSize), EDX);
It would be nice to name the register that is used for argument descriptor
(EDX).

[regis, 2012-08-30 22:45:31]

Thanks!

https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/ast.h#newco...
runtime/vm/ast.h:250: explicit ArgumentDefinitionTestNode(intptr_t token_pos,
On 2012/08/30 22:15:59, srdjan wrote:
> remove explicit

Oops. Done.

https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_ia32.cc:964: __ movl(Address(EBP, slot *
kWordSize), EDX);
On 2012/08/30 22:15:59, srdjan wrote:
> It would be nice to name the register that is used for argument descriptor
> (EDX).

For now, I named it locally (here and in the corresponding x64 location). We
would need to touch many files to be consistent and use global names for the
arguments descriptor, the name of the dynamic function, etc...

[hausner, 2012-08-31 00:34:55]

LGTM. Parser/scope code looks very delicate...

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/parser.c...
runtime/vm/parser.cc:7437: UNREACHABLE();
Is this really unreachable? What if the name refers to a global variable. You
still would not get an error while compiling the outermost function, but the
name would not be found in the parameter lists of the enclosing functions.

[...]

Now that i also read ParseArgumentDefinitionTest() I realize that is indeed true
that while parsing the inner function when the outer function got compiled, we
must have found the identifier in the chain of local scopes. This is really
delicate code ;)

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/scopes.c...
runtime/vm/scopes.cc:262: Symbols::Name(Symbols::kSavedEntryContextVar))) {
Isn't it enough to just allocate a string handle for the symbol, which gets
allocated as a RawString.

...Equals(String::Handle(Symbols::SavedEntryContextVar()))

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/symbols.cc
File runtime/vm/symbols.cc (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/symbols....
runtime/vm/symbols.cc:31: const char* Symbols::Name(intptr_t symbol) {
Why is this needed (or better) than getting the raw pointers using
Symbols::Foo()?

Symbols::Name(Symbols::kFoo) versus Symbols::Foo()

https://chromiumcodereview.appspot.com/10915022/diff/2004/tests/language/argu...
File tests/language/argument_definition_test.dart (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/tests/language/argu...
tests/language/argument_definition_test.dart:43: 
Can add this nice test function:

WTF([a, b, c]) {
  Expect.true(!?a??b:!?c == ?a?!?c:?b)
}

[regis, 2012-08-31 01:36:18]

Thanks!

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/parser.c...
runtime/vm/parser.cc:7437: UNREACHABLE();
On 2012/08/31 00:34:55, hausner wrote:
> Is this really unreachable? What if the name refers to a global variable. You
> still would not get an error while compiling the outermost function, but the
> name would not be found in the parameter lists of the enclosing functions.
> 
> [...]
> 
> Now that i also read ParseArgumentDefinitionTest() I realize that is indeed
true
> that while parsing the inner function when the outer function got compiled, we
> must have found the identifier in the chain of local scopes. This is really
> delicate code ;) 

Indeed, you missed the comment on line 7414 explaining why:
// 'ident' necessarily refers to the formal parameter of one of ...

Yes, the ?-operator is quite delicate. It refers to parameters without accessing
their values and therefore without capturing them.

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/scopes.c...
runtime/vm/scopes.cc:262: Symbols::Name(Symbols::kSavedEntryContextVar))) {
On 2012/08/31 00:34:55, hausner wrote:
> Isn't it enough to just allocate a string handle for the symbol, which gets
> allocated as a RawString.
> 
> ...Equals(String::Handle(Symbols::SavedEntryContextVar()))

Yes, but it seems like a waste to allocate all these handles in this recursive
function just to access a compile time char*. This is probably why you had the
static char* in the first place. I need also need a char* elsewhere for the
arg_desc variable name and I thought that making all these predefined symbols as
char* was convenient. But if it causes any issue, let's revisit.

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/symbols.cc
File runtime/vm/symbols.cc (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/symbols....
runtime/vm/symbols.cc:31: const char* Symbols::Name(intptr_t symbol) {
On 2012/08/31 00:34:55, hausner wrote:
> Why is this needed (or better) than getting the raw pointers using
> Symbols::Foo()?
> 
> Symbols::Name(Symbols::kFoo) versus Symbols::Foo()
>  

See the other comment. When we just need a char*, it is a waste to allocate a
handle.

https://chromiumcodereview.appspot.com/10915022/diff/2004/tests/language/argu...
File tests/language/argument_definition_test.dart (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/tests/language/argu...
tests/language/argument_definition_test.dart:43: 
On 2012/08/31 00:34:55, hausner wrote:
> Can add this nice test function:
> 
> WTF([a, b, c]) {
>   Expect.true(!?a??b:!?c == ?a?!?c:?b)
> }
> 

LOL
Does it work? Does it matter how you call WTF()?

[hausner, 2012-08-31 05:56:26]

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/scopes.c...
runtime/vm/scopes.cc:262: Symbols::Name(Symbols::kSavedEntryContextVar))) {
Ok, that's a valid reason that I buy. I hope Equals does not allocate a handle
(I don't have the source code handy).

On 2012/08/31 01:36:18, regis wrote:
> On 2012/08/31 00:34:55, hausner wrote:
> > Isn't it enough to just allocate a string handle for the symbol, which gets
> > allocated as a RawString.
> > 
> > ...Equals(String::Handle(Symbols::SavedEntryContextVar()))
> 
> Yes, but it seems like a waste to allocate all these handles in this recursive
> function just to access a compile time char*. This is probably why you had the
> static char* in the first place. I need also need a char* elsewhere for the
> arg_desc variable name and I thought that making all these predefined symbols
as
> char* was convenient. But if it causes any issue, let's revisit.

https://chromiumcodereview.appspot.com/10915022/diff/2004/tests/language/argu...
File tests/language/argument_definition_test.dart (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/tests/language/argu...
tests/language/argument_definition_test.dart:43: 
We had some good laughs in the office today. I haven't run this specific code,
but we did run similar ones and they worked. Feel free to use a different
function name :)

On 2012/08/31 01:36:18, regis wrote:
> On 2012/08/31 00:34:55, hausner wrote:
> > Can add this nice test function:
> > 
> > WTF([a, b, c]) {
> >   Expect.true(!?a??b:!?c == ?a?!?c:?b)
> > }
> > 
> 
> LOL
> Does it work? Does it matter how you call WTF()?