[Impeller] expose reference to tessellator instead of shared_ptr. #56244
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
looking good, just one c++ style comment
|
|
||
| std::shared_ptr<Tessellator> ContentContext::GetTessellator() const { | ||
| return tessellator_; | ||
| Tessellator& ContentContext::GetTessellator() const { |
There was a problem hiding this comment.
This should be const Tessellator& ContentContext::GetTessellator() const or Tessellator& ContentContext::GetTessellator()
https://google.github.io/styleguide/cppguide.html#Use_of_const
There was a problem hiding this comment.
tessellator methods are non const because it stores an allocation arena we use.
There was a problem hiding this comment.
So Tessellator& ContentContext::GetTessellator()?
There was a problem hiding this comment.
That's not what's there, you have a const method returning a non-const ref =)
There was a problem hiding this comment.
Declare methods to be const unless they alter the logical state of the object (or enable the user to modify that state, e.g., by returning a non-const reference, but that's rare), or they can't safely be invoked concurrently.
There was a problem hiding this comment.
I don't think that is what that means. The ContextVK is const, the tessellator is not. same with the host buffer?
There was a problem hiding this comment.
Constness should be transitive where if X is a aggregate of Y and Z and you have a const X, you should only be able to get a const Y& and const Z&.
There are already instances where this isn't the case here and admittedly Context is an odd grab bag of state. I was suggesting it if it's easy to do, otherwise this is fine since it doesn't regress what was already there.
There was a problem hiding this comment.
If that is an important policy to enforce, then it really needs to enforced via lints or the compiler.
|
|
||
| std::shared_ptr<Tessellator> ContentContext::GetTessellator() const { | ||
| return tessellator_; | ||
| Tessellator& ContentContext::GetTessellator() const { |
There was a problem hiding this comment.
Constness should be transitive where if X is a aggregate of Y and Z and you have a const X, you should only be able to get a const Y& and const Z&.
There are already instances where this isn't the case here and admittedly Context is an odd grab bag of state. I was suggesting it if it's easy to do, otherwise this is fine since it doesn't regress what was already there.
jonahwilliams
added
the
autosubmit
…157926) flutter/engine@c40b0b6...f2154ef 2024-10-31 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Skwasm single threaded (#56206)" (flutter/engine#56264) 2024-10-31 [email protected] Roll Skia from 4f8f2ecadfb6 to 3c628426f85f (1 revision) (flutter/engine#56261) 2024-10-31 [email protected] Roll Skia from 7e79a516284b to 4f8f2ecadfb6 (1 revision) (flutter/engine#56255) 2024-10-31 [email protected] Roll Dart SDK from 6a8058eef22c to f3e3dc44b1dc (1 revision) (flutter/engine#56253) 2024-10-31 [email protected] Roll Skia from 3c62d4a94d78 to 7e79a516284b (1 revision) (flutter/engine#56252) 2024-10-31 [email protected] [Impeller] use primitive restart for faster tessellation: write directly into host buffer. (flutter/engine#56173) 2024-10-31 [email protected] Skwasm single threaded (flutter/engine#56206) 2024-10-31 [email protected] [Impeller] expose reference to tessellator instead of shared_ptr. (flutter/engine#56244) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…utter/engine#56244) All geometries were incrementing the shared_ptr usage count which shows up in profiles. Instead expose a Tessellator reference like we do with HostBuffer.
2 participants
All geometries were incrementing the shared_ptr usage count which shows up in profiles. Instead expose a Tessellator reference like we do with HostBuffer.