commit f63c307c75549178d8c49eef79a823aca51cd6e4
author Ömer Sinan Ağacan <omersa@google.com> Thu Aug 25 14:06:40 2022 +0200
committer GitHub <noreply@github.com> Thu Aug 25 14:06:40 2022 +0200
tree 6adeca8ed4598e37b573997d0cb3ac10dadb9098
parent de8a92d71ff4f9061e30c2b3dce3236e2718dbdd

Refactor null handling in `_FieldSet` implementation (#739)

This PR refactors null handling in `_FieldSet` to avoid redundant null checks
(`!`) in the source code.

Generated JS is almost identical in the benchmark programs as dart2js does the
same transformation we do in this PR. For example, extension handling in
`_FieldSet.hashCode` is compiled to this JS in master branch:

```javascript
t1 = _this._extensions;
if (t1 != null) {
  t2 = t1._values;
  sortedByTagNumbers = A._sorted(new A.LinkedHashMapKeyIterable(t2, t2.$ti._eval$1("LinkedHashMapKeyIterable<1>")), type$.int);
  for (t3 = sortedByTagNumbers.length, t1 = t1._info, _i = 0; _i < sortedByTagNumbers.length; sortedByTagNumbers.length === t3 || (0, A.throwConcurrentModificationError)(sortedByTagNumbers), ++_i) {
    fi = t1.$index(0, sortedByTagNumbers[_i]);
    hash = A._FieldSet__hashField(hash, fi, t2.$index(0, fi.get$tagNumber()));
  }
}
```

With this PR:

```javascript
extensions = _this._extensions;
if (extensions != null) {
  t1 = extensions._values;
  sortedByTagNumbers = A._sorted(new A.LinkedHashMapKeyIterable(t1, t1.$ti._eval$1("LinkedHashMapKeyIterable<1>")), type$.int);
  for (t2 = sortedByTagNumbers.length, t3 = extensions._info, _i = 0; _i < sortedByTagNumbers.length; sortedByTagNumbers.length === t2 || (0, A.throwConcurrentModificationError)(sortedByTagNumbers), ++_i) {
    fi = t3.$index(0, sortedByTagNumbers[_i]);
    hash = A._FieldSet__hashField(hash, fi, t1.$index(0, fi.get$tagNumber()));
  }
}
```

This is the same as before, with `t1` renamed to `extensions`, and `t2` renamed
to `t1`.

This PR is still useful to avoid relying on compiler transformations, which
are often fragile and do not survive refactoring. Also, because each `!` can
potentially fail, having less of them is helpful when reading the code or
when refactoring.

Note that this pattern:

```dart
final eventPlugin = message._eventPlugin;
if (eventPlugin != null && eventPlugin.hasObservers) {
  ... use eventPlugin ...
}
```

Could be encapsulated in a `_FieldSet` method like:

```dart
void withEventPlugin(void Function(EventPlugin) f) {
  final eventPlugin = _eventPlugin;
  if (eventPlugin != null && eventPlugin.hasObservers) {
    f(eventPlugin);
  }
}
```

Which would simplify the use cases quite a bit as they all need the
`hasObservers` check.

However the performance implications are unclear to me, so I do not do this
refactoring in this PR.