Issue 62373009: Field property naming fix - issues 14096, 14806

Issue 62373009: Field property naming fix - issues 14096, 14806 (Closed)

Created:
7 years, 1 month ago by sra1

Modified:
7 years, 1 month ago

Reviewers:
ngeoffray

CC:
reviews_dartlang.org, ahe

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Fix issues with field names by ensuring that fields have exactly one name regardless of how the class is extended or mixed. R=ngeoffray@google.com Committed: https://code.google.com/p/dart/source/detail?r=30260

Patch Set 1 : #

Total comments: 14

Patch Set 2 : code review changes #

Created: 7 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+77 lines, -61 lines)			Patch
M	sdk/lib/_internal/compiler/implementation/elements/elements.dart	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/elements/modelx.dart	View	1	1 chunk	+7 lines, -7 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/js_backend/namer.dart	View	1	2 chunks	+56 lines, -19 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart	View	1	2 chunks	+2 lines, -14 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart	View		2 chunks	+2 lines, -4 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/ssa/codegen.dart	View		4 chunks	+3 lines, -11 lines	0 comments	Download
M	tests/compiler/dart2js_native/dart2js_native.status	View		1 chunk	+0 lines, -2 lines	0 comments	Download
M	tests/compiler/dart2js_native/native_mixin_field_test.dart	View		2 chunks	+3 lines, -2 lines	0 comments	Download
M	tests/language/language_dart2js.status	View	1	1 chunk	+3 lines, -1 line	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

ngeoffray

LGTM https://codereview.chromium.org/62373009/diff/60001/sdk/lib/_internal/compiler/implementation/js_backend/namer.dart File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right): https://codereview.chromium.org/62373009/diff/60001/sdk/lib/_internal/compiler/implementation/js_backend/namer.dart#newcode443 sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:443: String fieldPropertyName(Element element) { Please add a comment ...

7 years, 1 month ago (2013-11-11 10:31:32 UTC) #2

sra1

Committed patchset #2 manually as r30260 (presubmit successful).

7 years, 1 month ago (2013-11-14 00:25:27 UTC) #3

sra1

7 years, 1 month ago (2013-11-14 00:26:30 UTC) #4

Message was sent while issue was closed.

https://codereview.chromium.org/62373009/diff/60001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/62373009/diff/60001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:443: String
fieldPropertyName(Element element) {
On 2013/11/11 10:31:32, ngeoffray wrote:
> Please add a comment on the difference between fieldPropertyName and
> fieldAccessorName.

Done.

https://codereview.chromium.org/62373009/diff/60001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:444: if
(element.isInstanceMember()) return instanceFieldPropertyName(element);
On 2013/11/11 10:31:32, ngeoffray wrote:
> Use return ... ? ... : ...;

Done.

https://codereview.chromium.org/62373009/diff/60001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:449: if
(element.isInstanceMember()) return instanceFieldAccessorName(element);
On 2013/11/11 10:31:32, ngeoffray wrote:
> Ditto.

Done.

https://codereview.chromium.org/62373009/diff/60001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:466: if
(notShadowingAnotherField(element)) {
On 2013/11/11 10:31:32, ngeoffray wrote:
> Remove an if and use '&&' ?

I prefer not to write !a && b  when a is complex, so I restructured the
function.

https://codereview.chromium.org/62373009/diff/60001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:484: //
TODO(sra): Search entire chain to ensure not shadowing anther *field*.
On 2013/11/11 10:31:32, ngeoffray wrote:
> anter -> another

Done.

https://codereview.chromium.org/62373009/diff/60001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:486: // abstract
getter.
On 2013/11/11 10:31:32, ngeoffray wrote:
> What would it require to fix this?

Done.

https://codereview.chromium.org/62373009/diff/60001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:744: //
property?
On 2013/11/11 10:31:32, ngeoffray wrote:
> Remove TODO? I guess otherwise it does not pass any test.

Done.

Expand Messages | Collapse Messages