[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
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.
All geometries were incrementing the shared_ptr usage count which shows up in profiles. Instead expose a Tessellator reference like we do with HostBuffer.