Canonicalize TypeArguments.

Canonicalize TypeArguments.
Committed: https://code.google.com/p/dart/source/detail?r=2133

[regis, 2011-12-05 23:16:35]

LGTM with comments

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:2419: if (!type.IsNull() && !type.Equals(other_type)) {
What if type.IsNull() && !other_type.IsNull()?

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:2535: // type.IsNull() means it is Dynamic type.
We should not encounter a Null type in a non-null type arguments. Either the
vector is null or its elements are not null. We should debug when this case
arises.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:2625: RawAbstractTypeArguments*
TypeArguments::Canonicalize() const {
I would add a comment that Canonicalize for a non-instantiated type arguments is
a no-op.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:2633: intptr_t ix = 0;
index?

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:2659: raw_ptr()->is_canonical_ = value ? Bool::True() :
Bool::False();
Why do you store a dart value?
Other flags in classes and types are not dart values.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:5075: *FieldAddrAtOffset(field_offset) =
value.Canonicalize();
Same comment: What about inline assembly?

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.h#newcode...
runtime/vm/object.h:3083: raw_ptr()->type_arguments_ = value.Canonicalize();
I would make it more obvious that setting the TypeArguments will canonicalize
them.
What about setting them from inline assembly? Do they get canonicalized too?

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:6817: map_type_arguments = type_array.Canonicalize();
Is this necessary? You canonicalize below as well.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:6918: map_type_arguments,
Why not canonicalize here as well?

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:7167: type_arguments,
ditto

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:338: return
reinterpret_cast<RawObject**>(&ptr()->length_);
length_ needs to be changed if you keep a dart bool. But I would use a c++ bool
for is_canonical_.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:341: RawBool* is_canonical_;
Why a dart bool?

[srdjan, 2011-12-06 17:24:14]

Thanks for the comments! Also added checks that a canonical type argument cannot
be modified via SetTypeAt, SetLength.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:2419: if (!type.IsNull() && !type.Equals(other_type)) {
On 2011/12/05 23:16:35, regis wrote:
> What if type.IsNull() && !other_type.IsNull()?

Type can't be NULL, removed test (see comment below).

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:2535: // type.IsNull() means it is Dynamic type.
On 2011/12/05 23:16:35, regis wrote:
> We should not encounter a Null type in a non-null type arguments. Either the
> vector is null or its elements are not null. We should debug when this case
> arises.

Removed isNull case (reverting the lines here). It was occurring in some of my
test code that is now removed.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:2625: RawAbstractTypeArguments*
TypeArguments::Canonicalize() const {
On 2011/12/05 23:16:35, regis wrote:
> I would add a comment that Canonicalize for a non-instantiated type arguments
is
> a no-op.

Expanded the existing comment in header file.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:2633: intptr_t ix = 0;
On 2011/12/05 23:16:35, regis wrote:
> index?

Done.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:2659: raw_ptr()->is_canonical_ = value ? Bool::True() :
Bool::False();
On 2011/12/05 23:16:35, regis wrote:
> Why do you store a dart value?
> Other flags in classes and types are not dart values.

I am moving it to above the length of RawTypeArgument and converting it to bool.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:5075: *FieldAddrAtOffset(field_offset) =
value.Canonicalize();
On 2011/12/05 23:16:35, regis wrote:
> Same comment: What about inline assembly?

Discussed, not an issue.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/object.h#newcode...
runtime/vm/object.h:3083: raw_ptr()->type_arguments_ = value.Canonicalize();
On 2011/12/05 23:16:35, regis wrote:
> I would make it more obvious that setting the TypeArguments will canonicalize
> them.
> What about setting them from inline assembly? Do they get canonicalized too?

Discussed the inline assembly (went through code together), should not be a
problem.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:6817: map_type_arguments = type_array.Canonicalize();
On 2011/12/05 23:16:35, regis wrote:
> Is this necessary? You canonicalize below as well.

Removed, doing it later

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:6825: ASSERT(map_type_arguments.IsNull() ||
(map_type_arguments.Length() == 2));
Canonicalizing it here,

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:6918: map_type_arguments,
On 2011/12/05 23:16:35, regis wrote:
> Why not canonicalize here as well?

Done it above

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:6947:
AbstractTypeArguments::ZoneHandle(map_type_arguments.Canonicalize());
Reverted this.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:7167: type_arguments,
On 2011/12/05 23:16:35, regis wrote:
> ditto

Done by moving canonicalization before if (is_const)

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:338: return
reinterpret_cast<RawObject**>(&ptr()->length_);
On 2011/12/05 23:16:35, regis wrote:
> length_ needs to be changed if you keep a dart bool. But I would use a c++
bool
> for is_canonical_.

Removed Dart Bool, using C++ bool. I think length relates to types_ not to
length of the object.

http://codereview.chromium.org/8773026/diff/28002/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:341: RawBool* is_canonical_;
On 2011/12/05 23:16:35, regis wrote:
> Why a dart bool?

Changed to C++ bool.