feat: Type erased track and track state proxies #4889
Conversation
github-actions
bot
added
Component - Core
paulgessinger
changed the title
paulgessinger
force-pushed
the
feat/any-track
branch
from
d8708fd to
f18ce93
Compare
benjaminhuth
left a comment
benjaminhuth
left a comment
There was a problem hiding this comment.
Given the large PR I wonder if it would make sense to separate the usage of the constexpr hashes and the actual AnyTrack.
Apart from that I looks good to me, I have only a few comments.
| virtual ~TrackHandlerConstBase() = default; | ||
|
|
||
| /// Get the reference surface | ||
| virtual const Surface* referenceSurface(const void* container, |
There was a problem hiding this comment.
Hmm about the design choice to have a void * here. Have you also though if the container could be transferred via a std::any? This would provide more type safety, and I wonder if the performance implications would be measurable.
There was a problem hiding this comment.
Since this is internal API surface, and the consistency is guaranteed, I think void* is safe in this case.
I think the PR size is what it is because the track and track state API surface is (unavoidably) large. The hashes were mainly introduced to allow a more compact backend implementation (debatable if that worked, but that's a different question). In this PR, they help keeping the handler interface small: I can implement most of the public proxy functions internally via the component function, rather than having one virtual method for each public method. Is that what you're referring to, or am I missing the point? EDIT: Ah, you mean splitting up the changes to |
Yeah, thats what I meant. In general I also thought a bit if would make sense to only define the API surface once with some CRTP style pattern, but I was not sure if this in the end would improve code maintainability or make it just very difficult to understand whats going on... |
paulgessinger
force-pushed
the
feat/any-track
branch
from
4ed202b to
d081841
Compare
|
@benjaminhuth Yeah I was thinking about CRTP as well @benjaminhuth. One issue that I see is that the const-correctness handling is a bit delicate, which I think would be tricky to retain. I can give it a try though. |
paulgessinger
force-pushed
the
feat/any-track
branch
from
d081841 to
8ae692b
Compare
|
2 participants
This pull request introduces several improvements and refactorings to the event data proxy and track state handling system. The main focus is on enhancing type safety and code clarity by introducing named hashed string constants for column keys, updating concept definitions for proxies, and improving friend relationships and access patterns in proxy classes. It also adds new concepts and static assertions to ensure correct usage of proxy types throughout the codebase.
Type Safety and Code Clarity
TrackProxyandTrackStateProxy(e.g.,detail_tp::kTipIndexKey,detail_tsp::kPreviousKey) and refactored all usages to reference these constants, replacing direct calls tohashString("..."). This improves maintainability and reduces the risk of typos. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]Concepts and Type Constraints
TrackProxyLike,TrackProxyConcept,ConstTrackProxyConcept,MutableTrackProxyConcept) and updated their definitions to require specific methods and type aliases, improving compile-time guarantees and documentation of expected proxy behavior. [1] [2]TrackContainerto enforce that proxy types fulfill the required concepts.Proxy Accessor and Friend Relationships
ProxyAccessorBase::hasColumnto callproxy.hasColumn(key)instead of accessing the container directly, and removed the unusedassociatedConstProxytemplates.AnyTrack,AnyTrackState,TrackStateHandler), supporting future extensibility and type-safe access. [1] [2] [3]Minor Cleanups
MultiTrajectorymethods to streamline the code.These changes collectively improve the robustness, readability, and maintainability of the proxy system for event data, making it easier to extend and reason about in future development.
Blocked by: