Check types of case expressions

Check types of case expressions

Implement checks to make sure all case expressions are of the same type and other restrictions.

This change assumes that it's ok to use double values in switch expressions. Will verify with those who are in the know.

R=srdjan@google.com

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

[srdjan, 2013-09-26 18:35:42]

lgtm

https://codereview.chromium.org/24768004/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/24768004/diff/1/runtime/vm/parser.cc#newcode6047
runtime/vm/parser.cc:6047: void Parser::CheckCaseExpressions(const
GrowableArray<LiteralNode*>& values) {
Add brief comment what checks are done.

[hausner, 2013-09-26 18:54:19]

Committed patchset #3 manually as r27968 (presubmit successful).

[hausner, 2013-09-26 18:54:58]

https://codereview.chromium.org/24768004/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/24768004/diff/1/runtime/vm/parser.cc#newcode6047
runtime/vm/parser.cc:6047: void Parser::CheckCaseExpressions(const
GrowableArray<LiteralNode*>& values) {
On 2013/09/26 18:35:43, srdjan wrote:
> Add brief comment what checks are done.

Done.

[Ivan Posva, 2013-09-27 00:24:59]

PCU

Thanks,
-Ivan

https://codereview.chromium.org/24768004/diff/7001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/24768004/diff/7001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:6057: for (intptr_t i = 0; i < num_expressions; i++) {
Why don't you start with i = 1? Otherwise you just compare the first_value with
itself.

https://codereview.chromium.org/24768004/diff/7001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:6083: Resolver::ResolveDynamicAnyArgs(cls,
Symbols::EqualOperator()));
Can't you hoist this check out of the loop? Once you have determined that the
non-special class does not implement the operator == it should be fine for all
of them.

[hausner, 2013-09-27 00:31:34]

https://codereview.chromium.org/24768004/diff/7001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/24768004/diff/7001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:6057: for (intptr_t i = 0; i < num_expressions; i++) {
On 2013/09/27 00:24:59, Ivan Posva wrote:
> Why don't you start with i = 1? Otherwise you just compare the first_value
with
> itself.

Because I still need to check that the first value is either int, String or
double or doesn't implement ==. If there is only 1 case expression, that would
not get checked.

https://codereview.chromium.org/24768004/diff/7001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:6083: Resolver::ResolveDynamicAnyArgs(cls,
Symbols::EqualOperator()));
On 2013/09/27 00:24:59, Ivan Posva wrote:
> Can't you hoist this check out of the loop? Once you have determined that the
> non-special class does not implement the operator == it should be fine for all
> of them.

Good point. If I hoist it out I have to duplicate the int/String/double checks,
but I can skip this on subsequent iterations after it's checked once.

Will do in a separate checkin.

[Ivan Posva, 2013-09-27 00:44:43]

https://codereview.chromium.org/24768004/diff/7001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/24768004/diff/7001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:6083: Resolver::ResolveDynamicAnyArgs(cls,
Symbols::EqualOperator()));
On 2013/09/27 00:31:34, hausner wrote:
> On 2013/09/27 00:24:59, Ivan Posva wrote:
> > Can't you hoist this check out of the loop? Once you have determined that
the
> > non-special class does not implement the operator == it should be fine for
all
> > of them.
> 
> Good point. If I hoist it out I have to duplicate the int/String/double
checks,
> but I can skip this on subsequent iterations after it's checked once.
> 
> Will do in a separate checkin.

You can hoist the whole testing out. Most of the time the val.clazz() will be
first_value.clazz() even for int, strings and doubles. If for some reason that
is not the case (e.g. mixing one and two byte strings) it can be easily checked
in a slow case.

[hausner, 2013-09-27 00:49:16]

https://codereview.chromium.org/24768004/diff/7001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/24768004/diff/7001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:6083: Resolver::ResolveDynamicAnyArgs(cls,
Symbols::EqualOperator()));
On 2013/09/27 00:44:45, Ivan Posva wrote:
> On 2013/09/27 00:31:34, hausner wrote:
> > On 2013/09/27 00:24:59, Ivan Posva wrote:
> > > Can't you hoist this check out of the loop? Once you have determined that
> the
> > > non-special class does not implement the operator == it should be fine for
> all
> > > of them.
> > 
> > Good point. If I hoist it out I have to duplicate the int/String/double
> checks,
> > but I can skip this on subsequent iterations after it's checked once.
> > 
> > Will do in a separate checkin.
> 
> You can hoist the whole testing out. Most of the time the val.clazz() will be
> first_value.clazz() even for int, strings and doubles. If for some reason that
> is not the case (e.g. mixing one and two byte strings) it can be easily
checked
> in a slow case.

The whole reason why int and String are handled separately is because there are
several implentation classes: Smi, Mint, Integer, one-, two- and four-byte
Strings... Also, I think the way I check makes the error messages quite clear
(int expected, String expected, etc).

I think it's cleaner this way :)