[L] Change in dart/sdk[main]: [vm/shared] Skip runtime deep-immutability check for statically known...
Alexander Aprelev (Gerrit)
Alexander Aprelev voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 7 comments![]( "Open in Gerrit")
bool get hasDeeplyImmutableType;It seems like this is only applied to fields, so why do we need this getter on members?
bool requiresRuntimeCheck;Nit: these fields can be `final`.
_CheckResult({this.isImmutable = false, this.requiresRuntimeCheck = false}) {}Nit: this constructor can be `const`. Also, empty constructor body `{}` can be omitted (end with `;`).
field.hasDeeplyImmutableType =It seems like we only set this flag on instance fields of deeply-immutable classes (due to L91 and L151). But VM expects it on other fields (e.g. static).
Also, the value of this flag is determined from VM-specific `vm:deeply-immutable` pragmas. It would be cleaner and less intrusive to represent this information as a pragma instead of a flag on a `Field` object. Pragma can be added using
```
field.addAnnotation(ConstantExpression(InstanceConstant(coreTypes.pragmaClass.reference, [], {
coreTypes.pragmaName.fieldReference: StringConstant(pragmaName),
coreTypes.pragmaOptions.fieldReference: NullConstant(),
})))
```
requiresRuntimeCheck: false,Why runtime check is not need for arbitrary classes? Can we use type `Function` for a field?
field().has_deeply_immutable_type();Did you mean `!field().has_deeply_immutable_type()`? Because slow path which uses extra temp register is generated only if field doesn't have immutable type.
(also on other architectures)
final int baz;Could you add tests for shared static fields?
Also, consider writing IL tests for AOT compiler (https://github.com/dart-lang/sdk/blob/main/runtime/docs/infra/il_tests.md).
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 8 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
8 comments
It seems like this is only applied to fields, so why do we need this getter on members?
Done
Nit: these fields can be `final`.
Done
_CheckResult({this.isImmutable = false, this.requiresRuntimeCheck = false}) {}Nit: this constructor can be `const`. Also, empty constructor body `{}` can be omitted (end with `;`).
Done
It seems like we only set this flag on instance fields of deeply-immutable classes (due to L91 and L151). But VM expects it on other fields (e.g. static).
Also, the value of this flag is determined from VM-specific `vm:deeply-immutable` pragmas. It would be cleaner and less intrusive to represent this information as a pragma instead of a flag on a `Field` object. Pragma can be added using
```
field.addAnnotation(ConstantExpression(InstanceConstant(coreTypes.pragmaClass.reference, [], {
coreTypes.pragmaName.fieldReference: StringConstant(pragmaName),
coreTypes.pragmaOptions.fieldReference: NullConstant(),
})))
```
Done
Why runtime check is not need for arbitrary classes? Can we use type `Function` for a field?
Assumption here is that `isImmutable` is `true` only for statically proven classes.
Combination `(isImmutable: false, requiresRuntimeCheck: true)` doesn't make sense.
Did you mean `!field().has_deeply_immutable_type()`? Because slow path which uses extra temp register is generated only if field doesn't have immutable type.
(also on other architectures)
Done
Could you add tests for shared static fields?
Also, consider writing IL tests for AOT compiler (https://github.com/dart-lang/sdk/blob/main/runtime/docs/infra/il_tests.md).
Added one, but it's broken since that infra uses checked-in sdk. So will stay broken until checked-in sdk is updated.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |