Move the begin token of explicit functions to the start of the function declaration.

Move the begin token (token_pos) of explicit functions to the start of the function declaration.

This fixes setting the breakpoints when the signature of function declarations is spread across multiple lines. 

Example:
int
get
foo
=>
42;

BUG=
R=hausner@google.com

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

[hausner, 2013-08-13 15:40:11]

Good start, but not quite there yet I think. You have some cases that are not
consistent.

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/object_test.cc#...
runtime/vm/object_test.cc:3336: "  static void test1(int a) {\n"
Why did you change the functions to static? Is it because the class name is part
of the fingerprint? I think we lose a bit of test coverage here. We should have
a case that ensures that two instance functions that are equal except for the
class they belong to should NOT have the same fingerprint.

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/object_test.cc#...
runtime/vm/object_test.cc:3375: const Function& a_test1 =
Function::Handle(GetStaticFunction(
Nit: could you reformat this as

const Function& a_test1 =
    Function::Handle(GetStaticFunction(class_a, "test1"));

I think that's easier to read and more consistent with how we typically format
long lines.

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:1422: parameter.name_pos));
In the case of typedef, you change the token position of the signature function
to the beginning of the declaration. Here you leave the position unchanged to
point to the name, not the beginning of the type declaration. Should this not be
consistent?

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:5283: function_pos);
This case is not covered with your change. function_pos points to the opening
paren.

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:10066: if (lit->Equals("native")) {
I think you can speed this loop up by declaring a known symbol for "native" in
symbols.h. The literals in the token stream are guaranteed to be symbols, so you
can directly compare the raw pointers to detect the native pseudo-keyword.

Can we really have a library prefix named after a pseudo keyword? That's sick.

[Michael Lippautz (Google), 2013-08-13 18:07:34]

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/object_test.cc#...
runtime/vm/object_test.cc:3336: "  static void test1(int a) {\n"
On 2013/08/13 15:40:11, hausner wrote:
> Why did you change the functions to static? Is it because the class name is
part
> of the fingerprint? I think we lose a bit of test coverage here. We should
have
> a case that ensures that two instance functions that are equal except for the
> class they belong to should NOT have the same fingerprint.

Yes, it's because the fingerprint's intial value is the hashcode of the
signature (making it different for two classes A and B).

I added your proposed test case.

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/object_test.cc#...
runtime/vm/object_test.cc:3375: const Function& a_test1 =
Function::Handle(GetStaticFunction(
On 2013/08/13 15:40:11, hausner wrote:
> Nit: could you reformat this as
> 
> const Function& a_test1 =
>     Function::Handle(GetStaticFunction(class_a, "test1"));
> 
> I think that's easier to read and more consistent with how we typically format
> long lines.

Done.

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:1422: parameter.name_pos));
On 2013/08/13 15:40:11, hausner wrote:
> In the case of typedef, you change the token position of the signature
function
> to the beginning of the declaration. Here you leave the position unchanged to
> point to the name, not the beginning of the type declaration. Should this not
be
> consistent?

This is about closure parameters, so the beginning of the declaration is
actually the opening paren. I don't think we need to change this (?)

Reverted the change on typedef begin token though, since there's no need to have
it pointing at the begin of the declaration (for now).

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:5283: function_pos);
On 2013/08/13 15:40:11, hausner wrote:
> This case is not covered with your change. function_pos points to the opening
> paren.

Did miss this one. Done.

https://codereview.chromium.org/22916006/diff/3001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:10066: if (lit->Equals("native")) {
On 2013/08/13 15:40:11, hausner wrote:
> I think you can speed this loop up by declaring a known symbol for "native" in
> symbols.h. The literals in the token stream are guaranteed to be symbols, so
you
> can directly compare the raw pointers to detect the native pseudo-keyword.
> 

Done.

> Can we really have a library prefix named after a pseudo keyword? That's sick.
> 

Example:
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1039: 
native.NativeEnqueuer nativeCodegenEnqueuer(Enqueuer world)

grep -Ir "native\." sdk

https://codereview.chromium.org/22916006/diff/9001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/22916006/diff/9001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:10051: if (func.IsImplicitConstructor()) return;
Should this be hoisted out of the function and checked before it is called? All
the other Skip*() functions don't have parameters.

[hausner, 2013-08-13 19:29:52]

lgtm

https://codereview.chromium.org/22916006/diff/9001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/22916006/diff/9001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:10051: if (func.IsImplicitConstructor()) return;
On 2013/08/13 18:07:35, Michael Lippautz wrote:
> Should this be hoisted out of the function and checked before it is called?
All
> the other Skip*() functions don't have parameters.

Either way is fine. I see your point and maybe it would be nicer to do it at the
call site.

[Michael Lippautz (Google), 2013-08-13 20:11:31]

Committed patchset #4 manually as r26080 (presubmit successful).