Ensure that hash for a symbol is not set to 0 when it is made external.

Ensure that hash for a symbol is not set to 0 when it is made external.

R=iposva@google.com

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

[Ivan Posva, 2014-08-14 20:42:12]

LGTM
-Ivan

https://chromiumcodereview.appspot.com/474033002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/474033002/diff/1/runtime/vm/object.cc#...
runtime/vm/object.cc:17072: RELEASE_ASSERT(!result.HasHash() ||
We are aware that this makes the externalization of strings more expensive,
correct?

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

https://chromiumcodereview.appspot.com/474033002/diff/1/runtime/vm/object.h#n...
runtime/vm/object.h:5394: return (raw_ptr()->hash_ != NULL);
NULL? Shouldn't that be a Smi::Value(...) != 0?

[koda, 2014-08-14 20:43:30]

https://chromiumcodereview.appspot.com/474033002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/474033002/diff/1/runtime/vm/object.cc#...
runtime/vm/object.cc:17073: (result.Hash() == String::Hash(ext_array,
str_length)));
This is costly to always run in release mode, and we still have no guarantee it
will continue to be true (underlying bytes can change at any time).

[siva, 2014-08-14 21:04:57]

https://chromiumcodereview.appspot.com/474033002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/474033002/diff/1/runtime/vm/object.cc#...
runtime/vm/object.cc:17072: RELEASE_ASSERT(!result.HasHash() ||
On 2014/08/14 20:42:12, Ivan Posva wrote:
> We are aware that this makes the externalization of strings more expensive,
> correct?

Will change it to a plain ASSERT.

https://chromiumcodereview.appspot.com/474033002/diff/1/runtime/vm/object.cc#...
runtime/vm/object.cc:17073: (result.Hash() == String::Hash(ext_array,
str_length)));
On 2014/08/14 20:43:30, koda wrote:
> This is costly to always run in release mode, and we still have no guarantee
it
> will continue to be true (underlying bytes can change at any time).

See above changing to a plain ASSERT

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

https://chromiumcodereview.appspot.com/474033002/diff/1/runtime/vm/object.h#n...
runtime/vm/object.h:5394: return (raw_ptr()->hash_ != NULL);
On 2014/08/14 20:42:12, Ivan Posva wrote:
> NULL? Shouldn't that be a Smi::Value(...) != 0?

Let me change this in a CL by itself, I want to make sure it does
not have any performance side effects.

[siva, 2014-08-14 21:17:45]

Committed patchset #2 manually as r39268 (presubmit successful).