Fix for bug 1229: Unary operator plus is not allowed ... except for literals.

Fix for bug 1229: Unary operator plus is not allowed ... except for literals.
Add tests for incorrect literals, e.g. "+ 5".

Added a temporary token kTIGHTADD that represents an ADD not terminated by a whitespace. It is temporary and eliminated in the parser (code generator ot AST do not see it). It is used to recognize correct literals.
Committed: https://code.google.com/p/dart/source/detail?r=3745

[hausner, 2012-01-31 18:09:45]

At the very least the case +"foo" needs to be handled, but see my suggestion in
scanner.cc.

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/parser.cc...
runtime/vm/parser.cc:5692: if (!expr->IsLiteralNode()) {
This test is not strict enough. This also allows +"foo". You have to check for
number literals.

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/parser.cc...
runtime/vm/parser.cc:5693: ErrorMsg("'+' is not a valid prefix");
I wouldn't call + a prefix. How about "unexpected operator '+'" ? Also, please
print the error message at the token location of the +.

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/scanner.cc
File runtime/vm/scanner.cc (right):

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/scanner.c...
runtime/vm/scanner.cc:189: static bool IsWhiteSpace(char ch) {
This definition of what a whitespace character is differs from the one in the
function below. I find that confusing. I would inline the code in the one place
where it's used below, if you still need it (see comment below).

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/scanner.c...
runtime/vm/scanner.cc:603: IsWhiteSpace(c0_) ? Token::kADD : Token::kTIGHTADD;
Would it not make more sense to reverse the logic here? That is, a + is kADD
unless it is immediately followed by a digit and possibly a dot (+.5) or
whatever the rules for literals allow. In that case, we make it a kTIGHTADD.

https://chromiumcodereview.appspot.com/9264051/diff/1008/tests/corelib/src/Ma...
File tests/corelib/src/MathTest.dart (right):

https://chromiumcodereview.appspot.com/9264051/diff/1008/tests/corelib/src/Ma...
tests/corelib/src/MathTest.dart:233: Expect.equals(true,
parseIntThrowsBadNumberFormatException("+ 1"));
Already covered in line 229, but no harm.

https://chromiumcodereview.appspot.com/9264051/diff/1008/tests/language/src/L...
File tests/language/src/LiteralUnaryPlusNegativeTest.dart (right):

https://chromiumcodereview.appspot.com/9264051/diff/1008/tests/language/src/L...
tests/language/src/LiteralUnaryPlusNegativeTest.dart:8: var a = + 1;
Would be nice to cover the negative test cases 
a = + "foo";
a = +"foo"; 
var x = +a;
x = + a;

[srdjan, 2012-01-31 19:26:37]

Thanks for the comments. Adressed all, please have another look.

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/parser.cc...
runtime/vm/parser.cc:5692: if (!expr->IsLiteralNode()) {
On 2012/01/31 18:09:46, hausner wrote:
> This test is not strict enough. This also allows +"foo". You have to check for
> number literals.

Changed scanner to produce TIGHTADD only if the + is followed by a number
literal start. I.e., no extra test needed here (added comment here).

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/parser.cc...
runtime/vm/parser.cc:5693: ErrorMsg("'+' is not a valid prefix");
On 2012/01/31 18:09:46, hausner wrote:
> I wouldn't call + a prefix. How about "unexpected operator '+'" ? Also, please
> print the error message at the token location of the +.

Using "unexpected". The other option would have been "is not a valid prefix
operator".

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/scanner.cc
File runtime/vm/scanner.cc (right):

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/scanner.c...
runtime/vm/scanner.cc:189: static bool IsWhiteSpace(char ch) {
On 2012/01/31 18:09:46, hausner wrote:
> This definition of what a whitespace character is differs from the one in the
> function below. I find that confusing. I would inline the code in the one
place
> where it's used below, if you still need it (see comment below).

Discussed the difference offline. Removed the method since we are looking at
IsNumberStart instead.

https://chromiumcodereview.appspot.com/9264051/diff/1008/runtime/vm/scanner.c...
runtime/vm/scanner.cc:603: IsWhiteSpace(c0_) ? Token::kADD : Token::kTIGHTADD;
On 2012/01/31 18:09:46, hausner wrote:
> Would it not make more sense to reverse the logic here? That is, a + is kADD
> unless it is immediately followed by a digit and possibly a dot (+.5) or
> whatever the rules for literals allow. In that case, we make it a kTIGHTADD.

Changed.

https://chromiumcodereview.appspot.com/9264051/diff/1008/tests/language/src/L...
File tests/language/src/LiteralUnaryPlusNegativeTest.dart (right):

https://chromiumcodereview.appspot.com/9264051/diff/1008/tests/language/src/L...
tests/language/src/LiteralUnaryPlusNegativeTest.dart:8: var a = + 1;
On 2012/01/31 18:09:46, hausner wrote:
> Would be nice to cover the negative test cases 
> a = + "foo";
> a = +"foo"; 
> var x = +a;
> x = + a;
> 

Added in a new test (removed this one).

[hausner, 2012-01-31 19:57:09]

LGTM w/one nit.

https://chromiumcodereview.appspot.com/9264051/diff/12015/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9264051/diff/12015/runtime/vm/parser.c...
runtime/vm/parser.cc:5671: ErrorMsg("unexpected operator '+'");
Would still be nice to print the error at token position op_pos.

[srdjan, 2012-01-31 20:05:28]

https://chromiumcodereview.appspot.com/9264051/diff/12015/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9264051/diff/12015/runtime/vm/parser.c...
runtime/vm/parser.cc:5671: ErrorMsg("unexpected operator '+'");
On 2012/01/31 19:57:09, hausner wrote:
> Would still be nice to print the error at token position op_pos.

Done.