[M] Change in dart/sdk[main]: [vm/sdk] Use FinalThreadLocal for Iterable toString.
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
1 comment
if (!_toStringVisiting.isBound) {Alexander AprelevGiven that we introduced `FinalThreadLocal` anyway... Maybe you can rewrite this code to use that instead. And I think when you do that you can share the iterableToFullString and iterableToShortString methods back - it becomes platform independent.
Platform dependent part is `toStringVisiting`, which for VM becomes:
```
List get toStringVisiting => _toStringVisiting.value;
...
```And the rest of code can be shared.
Done
Related details
- Slava Egorov
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Slava Egorov
| Code-Review | +1 |
1 comment
for (int i = 0; i < toStringVisiting.length; i++) {Consider caching result of getter, e.g.
```
final visiting = toStringVisiting;
for (int i = 0; i < visiting.length; i++) {
if (identical(object, visiting[i])) return true;
}
return false;
```
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
2 comments
Consider caching result of getter, e.g.
```
final visiting = toStringVisiting;
for (int i = 0; i < visiting.length; i++) {
if (identical(object, visiting[i])) return true;
}
return false;
```
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Lasse Nielsen voted and added 8 comments![]( "Open in Gerrit")
Votes added by Lasse Nielsen
| Code-Review | -1 |
8 comments
Don't move map-function into `Iterable` and don't make the helper functions public.
const smallChangeThreshold = 13;(Arbitrary number just changed arbitrarily, for _reasons_! 😜)
static List<Object> get toStringVisiting => _toStringVisiting;That looks like it's just
```dart
@patch
static final List<Object> toStringVisiting = [];
```
with more steps. Unless that's not an allowed patch, then it's *necessary* more steps. Is it necessary?
static String mapToString(Map<Object?, Object?> m) => Iterable.mapToString(m);Don't move this function. It doesn't belong in `Iterable`.
static String mapToString(Map<Object?, Object?> m) {This is a new public function.
It used to be in `MapBase` and now it's in `Iterable`.
If it's no longer in `MapBase`, that's a breaking change, so don't remove it.
Also, it doesn't really belong in `Iterable`, maps are not `Iterable`, so why move it here?
If it stays, it should have a `@Since("3.11")`, or whatever version it goes into, and be documented in the CHANGELOG.md.
If you want to share it without needing to import `dart:collection`, then put it in `dart:_internal` and let `MapBase` reference it there.
static bool isToStringVisiting(Object object) {This should *probably* not be public.
external static List<Object> get toStringVisiting;This should _definitely_ not be public.
(Which tips the `toStringVisiting` towards not being public either.)
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |