[L] Change in dart/sdk[main]: [vm/immutable] Ensure that canonical instances are deeply-immutable.
Slava Egorov (Gerrit)
Slava Egorov voted and added 5 comments![]( "Open in Gerrit")
Votes added by Slava Egorov
| Code-Review | +1 |
5 comments
is_deeply_immutable_(is_deeply_immutable) {Given two asserts below consider removing this parameter altogether and just do:
```
is_deeply_immutable_(is_canonical_ || Object::ShouldHaveDeeplyImmutabilityBitSet(cid))
```
TypeParametersDeserializationCluster(bool is_canonical,There is assert below which says it is never canonical? So this is always false.
ASSERT(is_deeply_immutable);If you have an assert like this then pass constant `true` instead of the `is_deeply_immutable` variable to the constructor below and fold constant value into the constructor itself (if it is only invoked with one value.
case kFunctionTypeCid:I would imagine that these types are also deeply immutable? Should there be an assert? (Same for types below). If yes - please fold the constant into the constructor itself as described in the comment above.
ASSERT(is_deeply_immutable);Same comment as above. Fold constants into constructors everywhere - where appropriate.
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 added 2 comments![]( "Open in Gerrit")
is_deeply_immutable_(is_deeply_immutable) {Given two asserts below consider removing this parameter altogether and just do:
```
is_deeply_immutable_(is_canonical_ || Object::ShouldHaveDeeplyImmutabilityBitSet(cid))
```
Somehow in my mind by having explicit parameter and separate ASSERT I wanted to emphasize that here we consume `is_canonical` and `is_deeply_immutable` properties that were calculated at previous stages, only having an ASSERT to confirm assumptions that we generally have about these attributes.
Specifically I wanted to avoid use of `Object::ShouldHaveDeeplyImmutabilityBitSet` here which to me feels like leaking of implementation details if `is_deeply_immutable` proprety calculation.
TypeParametersDeserializationCluster(bool is_canonical,There is assert below which says it is never canonical? So this is always false.
Like above and below my intent with explicit parameter passing was to highlight the fact that code here doesn't calculate those `is_canonical`, `is_deeply_immutable` attributes, only consumes them.
Related details
- Slava Egorov
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 5 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
5 comments
Alexander AprelevGiven two asserts below consider removing this parameter altogether and just do:
```
is_deeply_immutable_(is_canonical_ || Object::ShouldHaveDeeplyImmutabilityBitSet(cid))
```
Somehow in my mind by having explicit parameter and separate ASSERT I wanted to emphasize that here we consume `is_canonical` and `is_deeply_immutable` properties that were calculated at previous stages, only having an ASSERT to confirm assumptions that we generally have about these attributes.
Specifically I wanted to avoid use of `Object::ShouldHaveDeeplyImmutabilityBitSet` here which to me feels like leaking of implementation details if `is_deeply_immutable` proprety calculation.
Done
TypeParametersDeserializationCluster(bool is_canonical,Alexander AprelevThere is assert below which says it is never canonical? So this is always false.
Like above and below my intent with explicit parameter passing was to highlight the fact that code here doesn't calculate those `is_canonical`, `is_deeply_immutable` attributes, only consumes them.
Also, this is plural TypeParameters, and assert is about singular TypeParameter.
If you have an assert like this then pass constant `true` instead of the `is_deeply_immutable` variable to the constructor below and fold constant value into the constructor itself (if it is only invoked with one value.
Done
I would imagine that these types are also deeply immutable? Should there be an assert? (Same for types below). If yes - please fold the constant into the constructor itself as described in the comment above.
Done
ASSERT(is_deeply_immutable);Same comment as above. Fold constants into constructors everywhere - where appropriate.
Which constant do you refer to in here?
Related details
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Slava Egorov
| Code-Review | +1 |
2 comments
ASSERT(is_deeply_immutable);Alexander AprelevSame comment as above. Fold constants into constructors everywhere - where appropriate.
Which constant do you refer to in here?
This one was wrong comment. I thought we had MintDeserializationCluster was taking is_deeply_immutable, but I see it is already folded in.
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 1 comment![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +2 |
1 comment
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
[vm/immutable] Ensure that canonical instances are deeply-immutable.
Fix snapshot serializer/deserializer to take care of deep-immutability proprety.
BUG=https://github.com/dart-lang/sdk/issues/62404
TEST=ci
- M runtime/vm/app_snapshot.cc
- M runtime/vm/class_id.h
- M runtime/vm/object.cc
Code-Review: +1 by Slava Egorov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |