Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1541)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 11478022: Implement is-checks with typedefs for callable objects. (Closed)

Created:
8 years ago by karlklose
Modified:
8 years ago
Reviewers:
floitsch, ngeoffray, kasperl
CC:
reviews_dartlang.org, floitsch
Visibility:
Public.

Description

Implement is-checks with typedefs for callable objects. This is not fixing all the tests in dartbug.com/5022, because these tests use static toplevel functions. BUG=dartbug.com/5507 Committed: https://code.google.com/p/dart/source/detail?r=15953

Patch Set 1 #

Patch Set 2 : #

Total comments: 6

Patch Set 3 : #

Total comments: 4

Patch Set 4 : Merge. #

Patch Set 5 : Address comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+58 lines, -36 lines) Patch
M sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart View 1 2 3 4 chunks +28 lines, -8 lines 0 comments Download
M sdk/lib/_internal/compiler/implementation/ssa/builder.dart View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M sdk/lib/_internal/compiler/implementation/ssa/codegen.dart View 1 2 3 4 1 chunk +2 lines, -4 lines 0 comments Download
M sdk/lib/_internal/compiler/implementation/ssa/nodes.dart View 1 2 1 chunk +1 line, -1 line 0 comments Download
M sdk/lib/_internal/compiler/implementation/ssa/optimize.dart View 1 2 3 1 chunk +3 lines, -1 line 0 comments Download
M tests/co19/co19-dart2dart.status View 1 2 3 4 1 chunk +11 lines, -0 lines 0 comments Download
M tests/co19/co19-dart2js.status View 1 2 3 4 4 chunks +11 lines, -20 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
karlklose
8 years ago (2012-12-07 11:58:49 UTC) #1
kasperl
First (trivial) comments: https://chromiumcodereview.appspot.com/11478022/diff/2001/tests/co19/co19-dart2dart.status File tests/co19/co19-dart2dart.status (right): https://chromiumcodereview.appspot.com/11478022/diff/2001/tests/co19/co19-dart2dart.status#newcode24 tests/co19/co19-dart2dart.status:24: Language/03_Overview/1_Scoping_A01_t39: Fail # http://dartbug/7202 http://dartbug/ -> ...
8 years ago (2012-12-07 12:01:20 UTC) #2
kasperl
LGTM. https://chromiumcodereview.appspot.com/11478022/diff/2001/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right): https://chromiumcodereview.appspot.com/11478022/diff/2001/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart#newcode2355 sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2355: if (identical(element, objectClass) || identical(element, compiler.dynamicClass)) { Long ...
8 years ago (2012-12-07 13:41:27 UTC) #3
floitsch
https://chromiumcodereview.appspot.com/11478022/diff/7001/sdk/lib/_internal/compiler/implementation/ssa/builder.dart File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right): https://chromiumcodereview.appspot.com/11478022/diff/7001/sdk/lib/_internal/compiler/implementation/ssa/builder.dart#newcode2744 sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2744: type.element.isClass()) || type.element.isTypeVariable()) { this looks too much like ...
8 years ago (2012-12-07 15:30:15 UTC) #4
ngeoffray
LGTM https://chromiumcodereview.appspot.com/11478022/diff/7001/sdk/lib/_internal/compiler/implementation/ssa/nodes.dart File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right): https://chromiumcodereview.appspot.com/11478022/diff/7001/sdk/lib/_internal/compiler/implementation/ssa/nodes.dart#newcode2544 sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2544: bool hasArgumentChecks() => inputs.length > 1; Is that ...
8 years ago (2012-12-10 07:07:54 UTC) #5
karlklose
8 years ago (2012-12-11 08:40:05 UTC) #6 
Thanks for the review.

https://codereview.chromium.org/11478022/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11478022/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2355: if
(identical(element, objectClass) || identical(element, compiler.dynamicClass)) {
On 2012/12/07 13:41:28, kasperl wrote:
> Long line. Not your fault.

Done.

https://codereview.chromium.org/11478022/diff/2001/tests/co19/co19-dart2dart....
File tests/co19/co19-dart2dart.status (right):

https://codereview.chromium.org/11478022/diff/2001/tests/co19/co19-dart2dart....
tests/co19/co19-dart2dart.status:24: Language/03_Overview/1_Scoping_A01_t39:
Fail # http://dartbug/7202
On 2012/12/07 12:01:20, kasperl wrote:
> http://dartbug/ -> http://dartbug.com/

Done.

https://codereview.chromium.org/11478022/diff/2001/tests/co19/co19-dart2js.st...
File tests/co19/co19-dart2js.status (right):

https://codereview.chromium.org/11478022/diff/2001/tests/co19/co19-dart2js.st...
tests/co19/co19-dart2js.status:599:
Language/14_Types/3_Type_Declarations/1_Typedef_A06_t01: Fail #
http://dartbug/7202
On 2012/12/07 12:01:20, kasperl wrote:
> http://dartbug/ -> http://dartbug.com/

Done.

https://codereview.chromium.org/11478022/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11478022/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2744:
type.element.isClass()) || type.element.isTypeVariable()) {
The change was actually not necessary anymore. Removed.

https://codereview.chromium.org/11478022/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/11478022/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2544: bool
hasArgumentChecks() => inputs.length > 1;
This does not show as a bug, because the code that uses it in the codegen will
iterate over zero elements instead of not iterating at all.

Powered by Google App Engine
This is Rietveld 408576698