When externalizing a string ensure that the basic tag bits are preserved.

When externalizing a string ensure that the basic tag bits are preserved.
Also make sure we do not try to externalize read only strings that are in
the VM isolate.
Committed: https://code.google.com/p/dart/source/detail?r=15553

[cshapiro, 2012-11-29 21:03:12]

lgtm with comments

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

https://codereview.chromium.org/11411204/diff/3001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:1753: if
(vm_isolate_heap->Contains(reinterpret_cast<uword>(str_obj.raw()))) {
Is this the most efficient check?  Can we use the canonical bit instead?

https://codereview.chromium.org/11411204/diff/3001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:1755: "cannot externalize a read only string.");
s/read only/read-only/

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

https://codereview.chromium.org/11411204/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:10501: NoGCScope no_gc;
Why did this move?  Was it scoped incorrectly before?  I like having the
NoGCScope be as narrow as possible to make it easy to understand the invariants
being preserved.

[siva, 2012-11-29 22:37:39]

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

https://codereview.chromium.org/11411204/diff/3001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:1753: if
(vm_isolate_heap->Contains(reinterpret_cast<uword>(str_obj.raw()))) {
My initial thought was strings in the isolate can also be canonical and checking
the canonical bit would essentially mean that all canonical strings cannot be
externalized.

As discussed off-line I will start conservatively by checking the canonical bit
and if it turns out to be
too restrictive we can re-evaluate.

On 2012/11/29 21:03:12, cshapiro wrote:
> Is this the most efficient check?  Can we use the canonical bit instead?

https://codereview.chromium.org/11411204/diff/3001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:1755: "cannot externalize a read only string.");
On 2012/11/29 21:03:12, cshapiro wrote:
> s/read only/read-only/

Done.

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

https://codereview.chromium.org/11411204/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:10501: NoGCScope no_gc;
It needs to be before we access any of the stuff from the object and cache in
local variables. We were caching
length, class id etc. and these could potentially change if somehow a GC is
triggered.

On 2012/11/29 21:03:12, cshapiro wrote:
> Why did this move?  Was it scoped incorrectly before?  I like having the
> NoGCScope be as narrow as possible to make it easy to understand the
invariants
> being preserved.

[Anton Muhin, 2012-11-30 21:37:45]

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

https://codereview.chromium.org/11411204/diff/7001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:1754: "cannot externalize a read-only string.");
might be worth clarifying what's the difference between immutable and read-only
:)