Error out on compile-time constants if equality is tested on bad types.

Error out on compile-time constants if equality is tested on bad types.

Don't fold equality operation if left-hand side is a constructed object.

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

[Lasse Reichstein Nielsen, 2012-03-27 08:12:12]

drive-by LGTM

https://chromiumcodereview.appspot.com/9866027/diff/1/frog/leg/compile_time_c...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9866027/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:903: if (left.isPrimitive() &&
right.isPrimitive()) {
Is "null" a numeric, string or boolean value? It seems null isn't included in
the spec allowances for compile-time constants comparisons.

https://chromiumcodereview.appspot.com/9866027/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:926: if (areIdentical === null) {
Have you considered having a dedicated FoldingFailed value you can return
instead of null. It would be infectous, so any operation using it would itself
be a failure (e.g., call it NaCTC :), and you could avoid null checks like this.

https://chromiumcodereview.appspot.com/9866027/diff/1/frog/leg/operations.dart
File frog/leg/operations.dart (right):

https://chromiumcodereview.appspot.com/9866027/diff/1/frog/leg/operations.dar...
frog/leg/operations.dart:244: } else if (!left.isConstructedObject()) {
I'd prefer 
  else if (left.isConstructedObject()) {
    return null;
  } else {
    ...
instead of the negative test. 
Generally, negative tests in if/else-if chains reduces readability *a lot*.

You can also drop the "else", since each branch returns.

[ngeoffray, 2012-03-27 11:14:34]

LGTM

[floitsch, 2012-03-28 00:25:21]

https://chromiumcodereview.appspot.com/9866027/diff/1/frog/leg/compile_time_c...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9866027/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:903: if (left.isPrimitive() &&
right.isPrimitive()) {
On 2012/03/27 08:12:12, Lasse Reichstein Nielsen wrote:
> Is "null" a numeric, string or boolean value? It seems null isn't included in
> the spec allowances for compile-time constants comparisons.

Filed already a bug, and Gilad confirmed that null should work too.

https://chromiumcodereview.appspot.com/9866027/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:926: if (areIdentical === null) {
On 2012/03/27 08:12:12, Lasse Reichstein Nielsen wrote:
> Have you considered having a dedicated FoldingFailed value you can return
> instead of null. It would be infectous, so any operation using it would itself
> be a failure (e.g., call it NaCTC :), and you could avoid null checks like
this.

I have considered it, but the problem is, that you want nice error-messages.
This means that there would need to be a way to attach a node to a Constant then
(since we don't want to work conditionally) which seems not so nice.
That said I'm not convinced that this is the cleanest it can get. I just think
that currently it is good enough and not too bad.

https://chromiumcodereview.appspot.com/9866027/diff/1/frog/leg/operations.dart
File frog/leg/operations.dart (right):

https://chromiumcodereview.appspot.com/9866027/diff/1/frog/leg/operations.dar...
frog/leg/operations.dart:244: } else if (!left.isConstructedObject()) {
On 2012/03/27 08:12:12, Lasse Reichstein Nielsen wrote:
> I'd prefer 
>   else if (left.isConstructedObject()) {
>     return null;
>   } else {
>     ...
> instead of the negative test. 
> Generally, negative tests in if/else-if chains reduces readability *a lot*.
> 
> You can also drop the "else", since each branch returns.

hehe. that's what I had before. After long hesitation I move the 'return null'
to the end. Reverted.
done.