Change layout of fields in a object values

Change layout of fields in a object values

BUG=
R=kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/1851ca685e5b936677510377bddbc8720dfe9d40

[zhivkag, 2017-03-27 09:32:02]

zhivkag@google.com changed reviewers:
+ kmillikin@google.com

[Kevin Millikin (Google), 2017-03-28 06:56:27]

LGTM with a few comments.

https://codereview.chromium.org/2781453002/diff/1/pkg/kernel/lib/interpreter/...
File pkg/kernel/lib/interpreter/interpreter.dart (right):

https://codereview.chromium.org/2781453002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:264: int get classSize =>
instanceFields.length;
This is more like instanceSize.

https://codereview.chromium.org/2781453002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:267: if
(_classes.containsKey(classRef)) {
This pattern is in the libraries as putIfAbsent so we could have:

return _classes.putIfAbsent(classRef,
    () => new Class._internal(classRef.asClass));

https://codereview.chromium.org/2781453002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:282: for (int i =
(superclass?.classSize ?? 0); i < instanceFields.length; i++) {
I think it makes sense to move this into _populateInstanceFields, so there we'd
have:

for (Field f in class_.fields) {
  if (f.isStatic) continue;
  instanceFields.add(f);
  assert(f.hasImplicitGetter);

  // Shadowing an inherited getter with the same name.
  getters[f.name] = ...;
  if (f.hasImplicitSetter) ...;
}

[zhivkag, 2017-03-28 09:18:14]

Thanks!

https://codereview.chromium.org/2781453002/diff/1/pkg/kernel/lib/interpreter/...
File pkg/kernel/lib/interpreter/interpreter.dart (right):

https://codereview.chromium.org/2781453002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:264: int get classSize =>
instanceFields.length;
On 2017/03/28 06:56:26, Kevin Millikin (Google) wrote:
> This is more like instanceSize.

Done.

https://codereview.chromium.org/2781453002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:267: if
(_classes.containsKey(classRef)) {
On 2017/03/28 06:56:26, Kevin Millikin (Google) wrote:
> This pattern is in the libraries as putIfAbsent so we could have:
> 
> return _classes.putIfAbsent(classRef,
>     () => new Class._internal(classRef.asClass));

Done.

https://codereview.chromium.org/2781453002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:282: for (int i =
(superclass?.classSize ?? 0); i < instanceFields.length; i++) {
On 2017/03/28 06:56:26, Kevin Millikin (Google) wrote:
> I think it makes sense to move this into _populateInstanceFields, so there
we'd
> have:
> 
> for (Field f in class_.fields) {
>   if (f.isStatic) continue;
>   instanceFields.add(f);
>   assert(f.hasImplicitGetter);
> 
>   // Shadowing an inherited getter with the same name.
>   getters[f.name] = ...;
>   if (f.hasImplicitSetter) ...;
> }

Done.

[zhivkag, 2017-03-28 09:19:27]

Description was changed from

==========
Change layout of fields in a object values

BUG=
==========

to

==========
Change layout of fields in a object values

BUG=
R=kmillikin@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1851ca685e5b936677510377bddbc8720dfe9d40
==========

[zhivkag, 2017-03-28 09:19:28]

Committed patchset #2 (id:20001) manually as
1851ca685e5b936677510377bddbc8720dfe9d40 (presubmit successful).