Save a handle allocation in two accessor functions of class Type.

Save a handle allocation in two accessor functions of class Type.

The ASSERT already makes sure, that we have the correct type at hand.
Committed: https://code.google.com/p/dart/source/detail?r=15775

[Florian Schneider, 2012-12-05 15:07:12]

Type::type_class shows up on the profile when running dart2js in checked mode.

[srdjan, 2012-12-05 20:02:44]

lgtm

[Ivan Posva, 2012-12-07 17:17:30]

Please come up with an alternative and safe proposal.

-Ivan

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

https://codereview.chromium.org/11444008/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:8715: return
reinterpret_cast<RawClass*>(raw_ptr()->type_class_);
I do not like the approach taken here. It should be safe in at least debug mode,
giving us the same protection which using the handle does. This pattern here
does not qualify.

https://codereview.chromium.org/11444008/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:8722: return
reinterpret_cast<RawUnresolvedClass*>(raw_ptr()->type_class_);
ditto + the check for HasResolvedTypeClass() has disappeared.

[Florian Schneider, 2012-12-10 16:11:27]

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

https://codereview.chromium.org/11444008/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:8715: return
reinterpret_cast<RawClass*>(raw_ptr()->type_class_);
On 2012/12/07 17:17:31, Ivan Posva wrote:
> I do not like the approach taken here. It should be safe in at least debug
mode,
> giving us the same protection which using the handle does. This pattern here
> does not qualify.

What protection do you want here? The caller has to wrap the raw pointer in a
handle anyway, so all asserts in handle-initialization will be done there. I
don't see much benefit of creating a handle here, just for asserting.

https://codereview.chromium.org/11444008/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:8722: return
reinterpret_cast<RawUnresolvedClass*>(raw_ptr()->type_class_);
On 2012/12/07 17:17:31, Ivan Posva wrote:
> ditto + the check for HasResolvedTypeClass() has disappeared.

Sure, I can re-insert the assert, but it is redundant wrt the asserts I added.

[Ivan Posva, 2012-12-10 17:13:42]

I will help you come up with an acceptable pattern for these kinds of
optimizations.

-Ivan

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

https://codereview.chromium.org/11444008/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:8715: return
reinterpret_cast<RawClass*>(raw_ptr()->type_class_);
On 2012/12/10 16:11:27, Florian Schneider wrote:
> On 2012/12/07 17:17:31, Ivan Posva wrote:
> > I do not like the approach taken here. It should be safe in at least debug
> mode,
> > giving us the same protection which using the handle does. This pattern here
> > does not qualify.
> 
> What protection do you want here? The caller has to wrap the raw pointer in a
> handle anyway, so all asserts in handle-initialization will be done there.

Please note that since you are returning a RawClass* here, that there is no
further protection (or checking) if the caller associates this with a Class
handle. This has been a deliberate decision, as we need to be able to trust the
return types of calls.

> I don't see much benefit of creating a handle here, just for asserting.

I did not say that you needed to create a handle here in the non-debug case.

https://codereview.chromium.org/11444008/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:8722: return
reinterpret_cast<RawUnresolvedClass*>(raw_ptr()->type_class_);
On 2012/12/10 16:11:27, Florian Schneider wrote:
> On 2012/12/07 17:17:31, Ivan Posva wrote:
> > ditto + the check for HasResolvedTypeClass() has disappeared.
> 
> Sure, I can re-insert the assert, but it is redundant wrt the asserts I added.

Type::HasResolvedType() conveys a completely different meaning from the
type_class_ field containing an object of type UnresolvedClass. It might be that
this is currently the case, but relying on that fact here makes me nervous.