Fix #28740 demangle constructors in stack traces (A.A becomes new A) REDO

Fix #28740 demangle constructors in stack traces (A.A becomes new A) REDO

Regular constructors and unnamed factory constructors:
- Were A.A, now are new A
Named constructors (including factory constructors):
- Were A.A.name, now are new A.name

Previous attempt (a84b6b7f) didn't update vm/cc tests, which broke the
build, was reverted, and also revealed an important case:

Functions within constructors:
- Were A.A.x, now are new A.x

Doing this was cleanest by switching the loop out for recursion, which
provides a nice guarantee that any anonymous function within some other
function will always have the correct prefix, and the class name (which
may or may not be prepended at the end) is only applied once, and its
existence is only checked in one place.

R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/a48e0ef493796f032aa0427124800917d6a1d75d

[mfairhurst, 2017-04-19 00:45:31]

mfairhurst@google.com changed reviewers:
+ asiva@google.com, rmacnak@google.com, vegorov@google.com

[mfairhurst, 2017-04-19 00:45:31]

Fun way to discover vm/cc tests: breaking the build!

Broken tests fixed, also revealed an interesting edge case. In the case that a
constructor contains a local function x, my previous attempt left that scenario
unchanged, ie, "A.A.x". That is inconsistent, so I updated it to be new "A.x".
However, that is also exactly what will be generated for a constructor name x on
class A.

Seems like the consistency is more important than the ambiguity, and the fact
that its an edge case makes the decision not as critical as otherwise. However,
there are solutions if we are willing to change the typical case.

For instance, we could go with "new A.x()" vs "new A().x", but that can be
confusing due to positional arguments (do we fill them? leave them?).

We could also go with "new A.local function x", but that gets very verbose when
nested: "new A.local function x.local function y.local function z" and doesn't
feel very solid.

Or we could go with "x (defined in A)" but that is backwards hierarchically, and
also gets verbose then looks like LISP "x (defined in y (defined in new A))"

So I vote to go with the simple, consistent approach that I've got here. If
anybody disagrees, I can happily brainstorm more ideas. And if anybody knows of
a reason why x within new A _must_ look different than new A.x, that will force
us to make the hard choice.

[siva, 2017-04-20 00:15:03]

https://codereview.chromium.org/2823253005/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2823253005/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7172: result =
String::Concat(String::Handle(fun.QualifiedName(name_visibility)),
Is it necessary to convert the iteration into a recursive call to QualifiedName
which again goes through all the checks and Handle creation etc.

https://codereview.chromium.org/2823253005/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7179: if (kind() == RawFunction::kConstructor) {
I tried your cl and changed this line to
if (fun.kind() == RawFunction::kConstructor) {

and the failing test works.

[mfairhurst, 2017-04-24 20:04:14]

On 2017/04/20 00:15:03, siva wrote:
> https://codereview.chromium.org/2823253005/diff/20001/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://codereview.chromium.org/2823253005/diff/20001/runtime/vm/object.cc#ne...
> runtime/vm/object.cc:7172: result =
> String::Concat(String::Handle(fun.QualifiedName(name_visibility)),
> Is it necessary to convert the iteration into a recursive call to
QualifiedName
> which again goes through all the checks and Handle creation etc.
> 
>
https://codereview.chromium.org/2823253005/diff/20001/runtime/vm/object.cc#ne...
> runtime/vm/object.cc:7179: if (kind() == RawFunction::kConstructor) {
> I tried your cl and changed this line to
> if (fun.kind() == RawFunction::kConstructor) {
> 
> and the failing test works.

Siva, definitely prefer your solution. Will do, once I get a chance.

[mfairhurst, 2017-04-25 23:16:48]

Done, followed Siva's recommendation to avoid recursion, definitely a better
solution.

[siva, 2017-04-26 16:49:27]

lgtm

[mfairhurst, 2017-04-28 17:19:25]

Description was changed from

==========
Fix #28740 demangle constructors in stack traces (A.A becomes new A) REDO

Regular constructors and unnamed factory constructors:
- Were A.A, now are new A
Named constructors (including factory constructors):
- Were A.A.name, now are new A.name

Previous attempt (a84b6b7f) didn't update vm/cc tests, which broke the
build, was reverted, and also revealed an important case:

Functions within constructors:
- Were A.A.x, now are new A.x

Doing this was cleanest by switching the loop out for recursion, which
provides a nice guarantee that any anonymous function within some other
function will always have the correct prefix, and the class name (which
may or may not be prepended at the end) is only applied once, and its
existence is only checked in one place.
==========

to

==========
Fix #28740 demangle constructors in stack traces (A.A becomes new A) REDO

Regular constructors and unnamed factory constructors:
- Were A.A, now are new A
Named constructors (including factory constructors):
- Were A.A.name, now are new A.name

Previous attempt (a84b6b7f) didn't update vm/cc tests, which broke the
build, was reverted, and also revealed an important case:

Functions within constructors:
- Were A.A.x, now are new A.x

Doing this was cleanest by switching the loop out for recursion, which
provides a nice guarantee that any anonymous function within some other
function will always have the correct prefix, and the class name (which
may or may not be prepended at the end) is only applied once, and its
existence is only checked in one place.

R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/a48e0ef493796f032aa0427124800917d6a1d75d
==========

[mfairhurst, 2017-04-28 17:19:26]

Committed patchset #3 (id:40001) manually as
a48e0ef493796f032aa0427124800917d6a1d75d (presubmit successful).