Add negation to single-identifier tests in status files.

Add negation to single-identifier tests in status files.

Optimize some regegps too.

Fixes #29756

BUG= http://dartbug.com/29756
R=rnystrom@google.com

Committed: https://github.com/dart-lang/sdk/commit/5720e3556c2fe8ae2f70eccbeeac5dfe85f9024f

[Lasse Reichstein Nielsen, 2017-05-31 09:57:52]

lrn@google.com changed reviewers:
+ whesse@google.com

[Bob Nystrom, 2017-06-01 23:07:45]

rnystrom@google.com changed reviewers:
+ rnystrom@google.com

[Bob Nystrom, 2017-06-01 23:07:46]

I was looking forward to doing this fun little change, but thank you either way.
:) LGTM after the following.

https://codereview.chromium.org/2913963002/diff/20001/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/2913963002/diff/20001/pkg/pkg.status#newcode94
pkg/pkg.status:94: [ $runtime == vm && $use_sdk ]
It would be super great to go through all the status files and remove uses of
the negated variables ("unchecked", "host_unchecked", etc.) Then remove support
for them in Environment.

Even better would also be to remove all uses of "== true", "== false", "!=
true", etc.

Even best would be to then remove support for comparing boolean variables to
true and false as literal values. Only allow them as direct variable
expressions, not comparisons.

Any of this could be left to later CLs, but TODOs would be nice.

https://codereview.chromium.org/2913963002/diff/20001/tests/standalone/status...
File tests/standalone/status_expression_test.dart (right):

https://codereview.chromium.org/2913963002/diff/20001/tests/standalone/status...
tests/standalone/status_expression_test.dart:87: var environment = <String,
dynamic>{
This map needs to be wrapped in new TestEnvironment() now that my change landed
yesterday. (Also, the type arguments aren't needed.)

https://codereview.chromium.org/2913963002/diff/20001/tests/standalone/status...
tests/standalone/status_expression_test.dart:89: "unchecked": false,
Nit: Would you mind using "checked" for this?

It seems a little confusing to use the negated term in the test when the whole
point of "!" is so we don't need negated terms.

https://codereview.chromium.org/2913963002/diff/20001/tests/standalone/status...
tests/standalone/status_expression_test.dart:96: environment["unchecked"] =
true;
"true" and "false" need to be explicitly stringified now. The environment
uniformly treats all values as strings.

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
File tools/testing/dart/status_expression.dart (right):

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
tools/testing/dart/status_expression.dart:200: bool negate = false;
Nit: "bool" -> "var".

I'm moving test.dart to consistently use var for locals. (Previously, it was
simply inconsistent.)

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
tools/testing/dart/status_expression.dart:223: var negate = _scanner.advance()
== _Token.notEqual;
How about renaming this local to "isNotEqual" to avoid confusing shadowing?

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
tools/testing/dart/status_expression.dart:242: static final _tokenPattern = new
RegExp(r"[()$]|&&|\|\||==|!=?|\w+");
Much nicer!

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
tools/testing/dart/status_expression.dart:276: current.length > 2 ||
_identifierPattern.hasMatch(current);
Is this an optimization? It seems unnecessary to me, unless this really does
address a visible (benchmarked?) performance problem.

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
File tools/testing/dart/status_file.dart (right):

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
tools/testing/dart/status_file.dart:75: if (hashIndex >= 0) {
Nit, I slightly prefer "!= -1" here. It's not like there are other possible
negative sentinel values.

[Lasse Reichstein Nielsen, 2017-06-02 08:38:18]

Description was changed from

==========
Add negation to single-identifier tests in status files.

Optimize some regegps too.

Fixes #29756

BUG= http://dartbug.com/29756
==========

to

==========
Add negation to single-identifier tests in status files.

Optimize some regegps too.

Fixes #29756

BUG= http://dartbug.com/29756
R=rnystrom@google.com

Committed:
https://github.com/dart-lang/sdk/commit/5720e3556c2fe8ae2f70eccbeeac5dfe85f9024f
==========

[Lasse Reichstein Nielsen, 2017-06-02 08:38:19]

Committed patchset #3 (id:40001) manually as
5720e3556c2fe8ae2f70eccbeeac5dfe85f9024f (presubmit successful).

[Lasse Reichstein Nielsen, 2017-06-07 08:51:50]

https://codereview.chromium.org/2913963002/diff/20001/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/2913963002/diff/20001/pkg/pkg.status#newcode94
pkg/pkg.status:94: [ $runtime == vm && $use_sdk ]
I'll just do it.
https://xkcd.com/208/

https://codereview.chromium.org/2913963002/diff/20001/tests/standalone/status...
File tests/standalone/status_expression_test.dart (right):

https://codereview.chromium.org/2913963002/diff/20001/tests/standalone/status...
tests/standalone/status_expression_test.dart:87: var environment = <String,
dynamic>{
On 2017/06/01 23:07:46, Bob Nystrom wrote:
> This map needs to be wrapped in new TestEnvironment() now that my change
landed
> yesterday. (Also, the type arguments aren't needed.)

Done.

https://codereview.chromium.org/2913963002/diff/20001/tests/standalone/status...
tests/standalone/status_expression_test.dart:89: "unchecked": false,
Sure. This was just the "minimal diff" from the test above :)

https://codereview.chromium.org/2913963002/diff/20001/tests/standalone/status...
tests/standalone/status_expression_test.dart:96: environment["unchecked"] =
true;
On 2017/06/01 23:07:45, Bob Nystrom wrote:
> "true" and "false" need to be explicitly stringified now. The environment
> uniformly treats all values as strings.

Done.

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
File tools/testing/dart/status_expression.dart (right):

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
tools/testing/dart/status_expression.dart:200: bool negate = false;
On 2017/06/01 23:07:46, Bob Nystrom wrote:
> Nit: "bool" -> "var".
> 
> I'm moving test.dart to consistently use var for locals. (Previously, it was
> simply inconsistent.)

Done.

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
tools/testing/dart/status_expression.dart:223: var negate = _scanner.advance()
== _Token.notEqual;
On 2017/06/01 23:07:46, Bob Nystrom wrote:
> How about renaming this local to "isNotEqual" to avoid confusing shadowing?

Done.

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
tools/testing/dart/status_expression.dart:242: static final _tokenPattern = new
RegExp(r"[()$]|&&|\|\||==|!=?|\w+");
Occupational hazard - I can't pass by a RegExp without needing to tweak it :)

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
tools/testing/dart/status_expression.dart:276: current.length > 2 ||
_identifierPattern.hasMatch(current);
Yes, optimization.
Necessary? Maybe not, but it seems ridiculously expensive to do full "^\w+$"
RegExp run on every identifier (which will, in almost all cases, match anyway)
when checking just the first character or even the length is sufficient.

I guess my approach to performance comes from library work - they need to be as
efficient as absolutely possible because they affect code that can't fix them. 
Applications can probably stop when they are "fast enough" because they can
predict all their uses.

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
File tools/testing/dart/status_file.dart (right):

https://codereview.chromium.org/2913963002/diff/20001/tools/testing/dart/stat...
tools/testing/dart/status_file.dart:75: if (hashIndex >= 0) {
There are two reasons for this choice (three if you include habit).
- I don't actually expect everybody to remember that the sentinel value is -1,
just that "negative means no". I see "-1" as a magic constant, and it's better
to avoid those.
- It's likely faster and smaller when compiled (pico-optimization, I know :)