-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Stats::Store should not inherit from Stats::Scope #23862
Description
I don't believe the "Store is a kind of Scope" inheritance fits the usual OO definitions of what inheritance should be. A Stats::Store object contains state across all the scopes (tls caches, histogram updating); it shouldn't really be a scope itself, IMO. It should contain a root scope, as well as the ability to iterate through the other scopes.
It's hard to tease this apart now, and until recently I was not motivated to try. However, when I changed Scope in #19791 to be managed by std::shared_ptr (with enable_shared_from_this for reasons) that can causes issues (exceptions thrown) in cases where a Store is instantiated directly rather than via std::make_shared or new, and that happens in tons of places, especially tests. There's no way for the compiler to enforce this given the inheritance, so we rely on tests to avoid getting into trouble. It's not a huge problem, but I just wanted to track this anyway.
I'm working in the background on cleaning this up in #23851 but I wanted to capture this issue to see if others have thoughts or have a strong preference for maintaining the "Store is a kind of Scope" relationship.