Avoid duplicate null checks when calling SetRaw from InitializeHandle

Avoid duplicate null checks when calling SetRaw from InitializeHandle
Committed: https://code.google.com/p/dart/source/detail?r=14589

[Ivan Posva, 2012-11-01 21:33:34]

I think there are other options for addressing the double checking. We should
first discuss the alternatives.

-Ivan

http://codereview.chromium.org/11369028/diff/3/vm/object.h
File vm/object.h (right):

http://codereview.chromium.org/11369028/diff/3/vm/object.h#newcode120
vm/object.h:120: obj->raw_ = Object::null();                                    
         \
We had discussed in the past whether this is the correct behaviour for null_
handles. Maybe we should reevaluate whether this check is really needed here.

http://codereview.chromium.org/11369028/diff/3/vm/object.h#newcode5661
vm/object.h:5661: if (raw_ == null_) {
The special case should not have been necessary in the original code if we set
the vtable of null_ to the Object vtable in the builtin_vtables_ list. That
might actually be a fine solution avoiding the split here.

[siva, 2012-11-01 21:59:53]

http://codereview.chromium.org/11369028/diff/3/vm/object.h
File vm/object.h (right):

http://codereview.chromium.org/11369028/diff/3/vm/object.h#newcode120
vm/object.h:120: obj->raw_ = Object::null();                                    
         \
I think we wanted to avoid the case where assignment of
a null_ would suddenly make Is##object() return false.

e.g:

String& str = String::Handle(...);
ASSERT(str.IsString());  // Valid.
str ^= String::null();
ASSERT(str.IsString());  // Still Valid.

If we don't do this check in initializeHandle we end up with
the second assert being invalid.

On 2012/11/01 21:33:34, Ivan Posva wrote:
> We had discussed in the past whether this is the correct behaviour for null_
> handles. Maybe we should reevaluate whether this check is really needed here.

http://codereview.chromium.org/11369028/diff/3/vm/object.h#newcode5661
vm/object.h:5661: if (raw_ == null_) {
the vtable of kNullCid in builtin_vtables_ you mean..

essentially:
  // Allocate and initialize the null class.
  cls = Class::New<Instance>(kNullCid);

should have been

  // Allocate and initialize the null class.
  cls = Class::New<Object>(kNullCid);

then yes we don't need this additional check and the
initializeHandle code can remain the way it is currently.


On 2012/11/01 21:33:34, Ivan Posva wrote:
> The special case should not have been necessary in the original code if we set
> the vtable of null_ to the Object vtable in the builtin_vtables_ list. That
> might actually be a fine solution avoiding the split here.

[Florian Schneider, 2012-11-01 22:35:36]

DBC:

http://codereview.chromium.org/11369028/diff/3/vm/object.h
File vm/object.h (right):

http://codereview.chromium.org/11369028/diff/3/vm/object.h#newcode116
vm/object.h:116: static void initializeHandle(object* obj, RawObject* raw_ptr) {
             \
How about adding a 

initializeHandle(object* obj, Raw##object* raw_ptr)

and 

Smi::SetRaw 

to avoid the smi-tag check that is currently in Object::SetRaw when creating a
Smi::Handle?

[siva, 2012-11-06 02:27:08]

As discussed offline, made the Object code similar to
all other handle types be having it call initializeHandle
instead of SetRaw directly. That way SetRaw does not need
the null check.

http://codereview.chromium.org/11369028/diff/3/vm/object.h
File vm/object.h (right):

http://codereview.chromium.org/11369028/diff/3/vm/object.h#newcode116
vm/object.h:116: static void initializeHandle(object* obj, RawObject* raw_ptr) {
             \
Will have to consider this in a different CL.

On 2012/11/01 22:35:36, Florian Schneider wrote:
> How about adding a 
> 
> initializeHandle(object* obj, Raw##object* raw_ptr)
> 
> and 
> 
> Smi::SetRaw 
> 
> to avoid the smi-tag check that is currently in Object::SetRaw when creating a
> Smi::Handle?

[Ivan Posva, 2012-11-06 03:35:18]

LGTM -ip