Avoid "=== true" inside code by calling a function that does this for us.

Avoid "=== true" inside code by calling a function that does this for us.

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

[floitsch, 2012-04-24 15:03:42]

Still looks quite hackish, and we should probably inline the functions in the
js_helpers.

[ngeoffray, 2012-04-25 11:48:08]

LGTM! I actually like it.

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (left):

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:99: return UNINTERCEPTED(a == b)
=== true;
The true comparison is evil here, could you write a test that shows it?

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:117: return UNINTERCEPTED(a ==
null) === true;
Remove === true, and same comment for test.

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:245: if (node.usedBy.length == 1
&& node.usedBy[0] is HBoolify) {
I'd check if all the users are HBoolify instead.

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:256: // Remove us from the usedBy
list of the old target.
Please move that code into the HInstruction class.

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:277: // This node stays the same,
but the Boolify node will go away.
Move that comment one line up (in the if clause).

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:288: return
visitInvokeBinary(node);
Should that be changed to super.visitEquals(node) ?

[floitsch, 2012-04-25 16:01:49]

PTAL.

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (left):

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:99: return UNINTERCEPTED(a == b)
=== true;
On 2012/04/25 11:48:08, ngeoffray wrote:
> The true comparison is evil here, could you write a test that shows it?

was on my agenda.
done.

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:117: return UNINTERCEPTED(a ==
null) === true;
On 2012/04/25 11:48:08, ngeoffray wrote:
> Remove === true, and same comment for test.

argh. good catch.
done.

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:245: if (node.usedBy.length == 1
&& node.usedBy[0] is HBoolify) {
On 2012/04/25 11:48:08, ngeoffray wrote:
> I'd check if all the users are HBoolify instead.

Done.

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:256: // Remove us from the usedBy
list of the old target.
On 2012/04/25 11:48:08, ngeoffray wrote:
> Please move that code into the HInstruction class.

Moved into HBasicBlock class.

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:277: // This node stays the same,
but the Boolify node will go away.
On 2012/04/25 11:48:08, ngeoffray wrote:
> Move that comment one line up (in the if clause).

Done.

https://chromiumcodereview.appspot.com/10140027/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:288: return
visitInvokeBinary(node);
On 2012/04/25 11:48:08, ngeoffray wrote:
> Should that be changed to super.visitEquals(node) ?

Done.

[floitsch, 2012-04-25 19:53:21]

small fixes.

[ngeoffray, 2012-04-26 08:40:00]

LGTM

https://chromiumcodereview.appspot.com/10140027/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10140027/diff/13001/lib/compiler/imple...
lib/compiler/implementation/ssa/nodes.dart:593: static void
removeUser(HInstruction instruction, HInstruction user) {
Why a static function and not a function on HInstruction?

https://chromiumcodereview.appspot.com/10140027/diff/13001/tests/language/src...
File tests/language/src/Operator5Test.dart (right):

https://chromiumcodereview.appspot.com/10140027/diff/13001/tests/language/src...
tests/language/src/Operator5Test.dart:1: // Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
2011 -> 2012

[floitsch, 2012-04-26 10:36:09]

https://chromiumcodereview.appspot.com/10140027/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10140027/diff/13001/lib/compiler/imple...
lib/compiler/implementation/ssa/nodes.dart:593: static void
removeUser(HInstruction instruction, HInstruction user) {
On 2012/04/26 08:40:00, ngeoffray wrote:
> Why a static function and not a function on HInstruction?

Will change in a new CL.

https://chromiumcodereview.appspot.com/10140027/diff/13001/tests/language/src...
File tests/language/src/Operator5Test.dart (right):

https://chromiumcodereview.appspot.com/10140027/diff/13001/tests/language/src...
tests/language/src/Operator5Test.dart:1: // Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
On 2012/04/26 08:40:00, ngeoffray wrote:
> 2011 -> 2012

Done.

[ngeoffray, 2012-04-27 11:25:14]

LGTM!