Add Double::NewCanonical and Mint::NewCanonical so that it is possible to

Add Double::NewCanonical and Mint::NewCanonical so that it is possible to
canonicalize double and mint constants without having to create an object first.
Committed: https://code.google.com/p/dart/source/detail?r=2496

[srdjan, 2011-12-15 22:02:26]

LGTM with comments and suggestions

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc
File vm/object.cc (right):

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5017
vm/object.cc:5017: const Instance& constant) {
Why not make it a Class function?

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5020
vm/object.cc:5020: if (index == list_len) {
index >= list_len ?

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5021
vm/object.cc:5021: const intptr_t new_length = (list_len == 0) ? 4 : list_len *
2;
That grows a little bit too quickly, IMHO.

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5034
vm/object.cc:5034: if (!IsCanonical()) {
Can we move all this code into Class, thus no access here to Class::constants()?

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5395
vm/object.cc:5395: ASSERT(!Smi::IsValid64(value));
If we move this code into Class, we could share it with others?

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5397
vm/object.cc:5397:
Class::Handle(Isolate::Current()->object_store()->mint_class());
Isn't cls = Class::Handle(clazz()); ?

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5499
vm/object.cc:5499: }
Can you use (*reinterpret_cast<int64_t*>(&value) to do the bitwise compare?

http://codereview.chromium.org/8963001/diff/3002/vm/object.h
File vm/object.h (right):

http://codereview.chromium.org/8963001/diff/3002/vm/object.h#newcode2437
vm/object.h:2437: bool Equals(double value) const;
Please give this one a different name, e.g. EqualsToDouble, EqualsValue...

http://codereview.chromium.org/8963001/diff/3002/vm/object.h#newcode2444
vm/object.h:2444: static RawDouble* NewCanonical(double d);
Please use different names, e.g., NewCanonicalFromDouble,
NewCanonicalFromString.

http://codereview.chromium.org/8963001/diff/3002/vm/object.h#newcode2446
vm/object.h:2446: static RawDouble* NewCanonical(const String& str);
Need to comment that a Double::null() is returned if str does not contain a
double.

[siva, 2011-12-15 23:42:54]

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc
File vm/object.cc (right):

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5017
vm/object.cc:5017: const Instance& constant) {
On 2011/12/15 22:02:26, srdjan wrote:
> Why not make it a Class function?

Done.

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5020
vm/object.cc:5020: if (index == list_len) {
On 2011/12/15 22:02:26, srdjan wrote:
> index >= list_len ?

Done.

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5021
vm/object.cc:5021: const intptr_t new_length = (list_len == 0) ? 4 : list_len *
2;
Changed it to (list_len + 4) for growth, so grow by 4 elements a time.

On 2011/12/15 22:02:26, srdjan wrote:
> That grows a little bit too quickly, IMHO.

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5034
vm/object.cc:5034: if (!IsCanonical()) {
As discussed off-line, I have added a TODO to consider
refactoring this so thath we can effectively share for
all look ups.

On 2011/12/15 22:02:26, srdjan wrote:
> Can we move all this code into Class, thus no access here to
Class::constants()?

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5395
vm/object.cc:5395: ASSERT(!Smi::IsValid64(value));
Ditto.

On 2011/12/15 22:02:26, srdjan wrote:
> If we move this code into Class, we could share it with others?

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5397
vm/object.cc:5397:
Class::Handle(Isolate::Current()->object_store()->mint_class());
This is a static method, clazz() is not valid.

On 2011/12/15 22:02:26, srdjan wrote:
> Isn't cls = Class::Handle(clazz()); ?

http://codereview.chromium.org/8963001/diff/3002/vm/object.cc#newcode5499
vm/object.cc:5499: }
If I do that I get :
error: dereferencing type-punned pointer will break strict-aliasing rules

On 2011/12/15 22:02:26, srdjan wrote:
> Can you use (*reinterpret_cast<int64_t*>(&value) to do the bitwise compare?

http://codereview.chromium.org/8963001/diff/3002/vm/object.h
File vm/object.h (right):

http://codereview.chromium.org/8963001/diff/3002/vm/object.h#newcode2437
vm/object.h:2437: bool Equals(double value) const;
On 2011/12/15 22:02:26, srdjan wrote:
> Please give this one a different name, e.g. EqualsToDouble, EqualsValue...

Done.

http://codereview.chromium.org/8963001/diff/3002/vm/object.h#newcode2444
vm/object.h:2444: static RawDouble* NewCanonical(double d);
This style seems to go against the style we have used for New(...) methods
above.

On 2011/12/15 22:02:26, srdjan wrote:
> Please use different names, e.g., NewCanonicalFromDouble,
> NewCanonicalFromString.

http://codereview.chromium.org/8963001/diff/3002/vm/object.h#newcode2446
vm/object.h:2446: static RawDouble* NewCanonical(const String& str);
On 2011/12/15 22:02:26, srdjan wrote:
> Need to comment that a Double::null() is returned if str does not contain a
> double.

Done.