Lookup functions by name that contains the private key, except for dart_api which allows ignoring t…

Lookup functions by name that contains the private key, except for dart_api which allows ignoring the private key.
Committed: https://code.google.com/p/dart/source/detail?r=17178

[siva, 2013-01-16 23:57:24]

lgtm

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

https://codereview.chromium.org/11968022/diff/2002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:5763: // TODO(srdjan): Improve speed by assuming
OnebyteStrings.
could also be ExternalOneByteString?

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:1109:
String::Handle(core_lib.PrivateName(Symbols::_get_or_create()));
Can we add a TODO here that private names in the symbol table need to be stored
with the private key.

That will avoid the use of the function Library::PrivateName() which does not
seem to be set up for qualified names (see comment in that file).

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

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/object.cc#newco...
runtime/vm/object.cc:5742: static bool ShouldBePrivate(const String& name) {
I find the name ShouldBePrivate pretty weird.
It seems to be doing the functionality of
IsPrivate(const String& name);

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/object.cc#newco...
runtime/vm/object.cc:5750: // TODO(srdjan): Improve speed by assuming
OnebyteStrings.
assuming OneByteStrings or ExternalOneByteStrings

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/object.cc#newco...
runtime/vm/object.cc:5757: }
As discussed offline, this additional check here probably only makes sense for
Library::PrivateName().

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6258: return str.raw();
I think the function in it's current form will not work for
cases like _xyz.func
I don't think this function was written to accept qualified names.

it is supposed to make it _xyz@123.func but it will make it _xyz.func@123

[Ivan Posva, 2013-01-17 00:25:14]

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:1109:
String::Handle(core_lib.PrivateName(Symbols::_get_or_create()));
Using the core_lib private key does not seem appropriate here. Is this being
tested?

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/dart_entry.cc#n...
runtime/vm/dart_entry.cc:152:
core_lib.LookupClassAllowPrivate(Symbols::InvocationMirror()));
Why are we allowing private here?

[srdjan, 2013-01-17 00:31:11]

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

https://codereview.chromium.org/11968022/diff/2002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:5763: // TODO(srdjan): Improve speed by assuming
OnebyteStrings.
On 2013/01/16 23:57:24, siva wrote:
> could also be ExternalOneByteString?

Removed comment.

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:1109:
String::Handle(core_lib.PrivateName(Symbols::_get_or_create()));
On 2013/01/16 23:57:24, siva wrote:
> Can we add a TODO here that private names in the symbol table need to be
stored
> with the private key.
> 
> That will avoid the use of the function Library::PrivateName() which does not
> seem to be set up for qualified names (see comment in that file).

Done.

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:1109:
String::Handle(core_lib.PrivateName(Symbols::_get_or_create()));
On 2013/01/17 00:25:14, Ivan Posva wrote:
> Using the core_lib private key does not seem appropriate here. Is this being
> tested?

There are no tests apparently . Fixed, thanks for spotting it.

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/dart_entry.cc#n...
runtime/vm/dart_entry.cc:152:
core_lib.LookupClassAllowPrivate(Symbols::InvocationMirror()));
On 2013/01/17 00:25:14, Ivan Posva wrote:
> Why are we allowing private here?

Changed to use private key.

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

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/object.cc#newco...
runtime/vm/object.cc:5742: static bool ShouldBePrivate(const String& name) {
On 2013/01/16 23:57:24, siva wrote:
> I find the name ShouldBePrivate pretty weird.
> It seems to be doing the functionality of
> IsPrivate(const String& name);

Reverting this code to original, adding an IsPrivate method that does the same
(used in an ASSERT of Library::PrivateName)

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/object.cc#newco...
runtime/vm/object.cc:5750: // TODO(srdjan): Improve speed by assuming
OnebyteStrings.
On 2013/01/16 23:57:24, siva wrote:
> assuming OneByteStrings or ExternalOneByteStrings

Done.

https://codereview.chromium.org/11968022/diff/4013/runtime/vm/object.cc#newco...
runtime/vm/object.cc:5757: }
On 2013/01/16 23:57:24, siva wrote:
> As discussed offline, this additional check here probably only makes sense for
> Library::PrivateName().

Done.