Report NON_CONSTANT_VALUE_IN_INITIALIZER

Report NON_CONSTANT_VALUE_IN_INITIALIZER

R=brianwilkerson@google.com
BUG=

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

[Brian Wilkerson, 2013-05-14 15:49:27]

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java
(right):

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java:74:
private static boolean isBoolOrNull(ValidResult result) {
Why aren't these 'is' methods instance methods?

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java:75:
return result == RESULT_BOOL || result == RESULT_NULL;
Does this want to include RESULT_FALSE and RESULT_TRUE?

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java:113:
if (isIntOrNull(this)) {
Wouldn't null result in an error (here and in the other unary operator methods
below)?

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java:279:
if (isNumOrNull(this) || isNumOrNull(leftOperand)) {
Again, it seems like we're missing some error conditions here. The code seems to
be assuming that we won't have an expression in which one operand is a num and
the other a String (or other constant) that should result in an error.

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java
(right):

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:345:
private void validateInitializerIntivationArguments(ParameterElement[]
parameterElements,
nit: mistyped method name

[scheglov, 2013-05-14 23:51:06]

PTAL

There are failing tests in co19, I've opened an issue.

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java
(right):

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java:74:
private static boolean isBoolOrNull(ValidResult result) {
On 2013/05/14 15:49:28, Brian Wilkerson wrote:
> Why aren't these 'is' methods instance methods?

Done.

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java:75:
return result == RESULT_BOOL || result == RESULT_NULL;
On 2013/05/14 15:49:28, Brian Wilkerson wrote:
> Does this want to include RESULT_FALSE and RESULT_TRUE?

No.
We use isBool() to check that ValidResult is some unknown boolean, so that we
will not evaluate it.
If there is an actual value, it will be evaluated as usually.

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java:113:
if (isIntOrNull(this)) {
On 2013/05/14 15:49:28, Brian Wilkerson wrote:
> Wouldn't null result in an error (here and in the other unary operator methods
> below)?

Yes.
Now we will report CONST_EVAL_THROWS_EXCEPTION for null here.

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java:279:
if (isNumOrNull(this) || isNumOrNull(leftOperand)) {
On 2013/05/14 15:49:28, Brian Wilkerson wrote:
> Again, it seems like we're missing some error conditions here. The code seems
to
> be assuming that we won't have an expression in which one operand is a num and
> the other a String (or other constant) that should result in an error.

I've added more checks.

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java
(right):

https://codereview.chromium.org/15096008/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:345:
private void validateInitializerIntivationArguments(ParameterElement[]
parameterElements,
On 2013/05/14 15:49:28, Brian Wilkerson wrote:
> nit: mistyped method name

Done.

[Brian Wilkerson, 2013-05-15 04:13:49]

With one minor change, LGTM.

https://codereview.chromium.org/15096008/diff/6001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java
(right):

https://codereview.chromium.org/15096008/diff/6001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java:959:
private boolean isBool2() {
I really don't like using "2" as a suffix; it lacks any semantic meaning.
Perhaps names like "isPotentialBool" and "isAnyBool" would be better (I'm also
open to other suggestions.)

[scheglov, 2013-05-15 16:59:03]

https://codereview.chromium.org/15096008/diff/6001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java
(right):

https://codereview.chromium.org/15096008/diff/6001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ValidResult.java:959:
private boolean isBool2() {
On 2013/05/15 04:13:49, Brian Wilkerson wrote:
> I really don't like using "2" as a suffix; it lacks any semantic meaning.
> Perhaps names like "isPotentialBool" and "isAnyBool" would be better (I'm also
> open to other suggestions.)

Done.

[scheglov, 2013-05-15 16:59:25]

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