-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Add a generic variant based type that can be used for interface-like functionality #215
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Only necessary to be non-default in case of relation handling
Necessary to use OneToOneRelations in a different namespace
Interface types can only use Types that are defined in the datatypes section. datatypes can only use interface types as relations (same as with other datatypes). Members of interface types are not yet checked.
Previously the getValue call made it possible to create a GenericWrapper from a Const data type but get a mutable one out
Make the all possible GenericWrappers friends of the user facing classes. This gives the wrappers the necessary access, while still keeping everyone else from getting its hand on the Obj*
Also make sure that using types from different namespaces is possible in interface types
tmadlener
force-pushed
the
generic-wrappers
branch
from
September 10, 2021 12:19
5ad307b
to
6e83818
Compare
@tmadlener - would be great to actually see the foreseen user code use cases and improvements |
3 tasks
superseded by #516 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BEGINRELEASENOTES
podio::GenericWrapper
class that can wrap multiple user data types via an underlyingstd::variant
.interface
section to the datamodel definition where "interface" classes can be defined. These datatypes use thepodio::GenericWrapper
and can store multiple different user datatypes that all offer similar functionality and serve as a sort of "base type" that can then be used as any other datatype.ENDRELEASENOTES
Contains all changes from #143 and #217
Rationale
It is possible that there are specialized datatypes that all offer very similar functionality, e.g. in LCIO there is a whole hierarchy of
TrackerHit
classes, which means that e.g. EDM4hep will also potentially need more than oneTrackerHit
class (see key4hep/EDM4hep#121). Adding more datatypes is quite trivial (e.g. key4hep/EDM4hep#122). However, it is currently not possible to have a heterogeneous collection of different datatypes in podio generated datamodels. Hence, e.g. a "simple"Track
class, which can store different types ofTrackerHit
s within a singleOneToManyRelation
is not possible and a definition would have to look like this:This makes for a rather awkward usage as all the possible relations have to be checked and it is not easily possible to simply loop over all tracker hits, regardless of their type. Additionally, every time a new tracker hit type is added all user code that does such a loop would have to be changed, to now also include the new type (arguably such additions would hopefully be few, but even once might be enough already)
Proposed solution
podio::GenericWrappers
The proposed implementation solves this problem by introducing a
podio::GenericWrapper
class, that on a very basic level looks like this (not strictly valid c++, just for illustrating the point, see the code for details)Using the internal
std::variant
theGenericWrappers
class offers some basic functionality that is expected from all user-facing objects (e.g.getObjectID
), viastd::visit
. Additionally, it offers functionality to probe what the currently held type is, as well as functionality to try and get the "actual" type from the generic wrapper (if it is currently held). These are the two functions:With this class users can already do something like
Interface types
The
GenericWrapper
class allows to store different tracker hit classes in one type. However, its functionality is (deliberately) limited and to do something useful it is still necessary to cast back to the original tracker hit type. To solve this, additional wrapper classes are introduced to the generated code, which inherit frompodio::GenericWrapper
. They are generated from datatype definitions in a new section of the datamodel yaml (taking edm4hep and LCIO here, but there is also another example in the PR for the podio example datamodel)For all datatypes in the
interfaces
section user facing classes are generated that behave exactly the same as other datamodel classes, with the only difference, that they have to be initialized with another value (they cannot be default constructed). TheMembers
section of each type defines the publicly available setter and getter functions on these "interface" types. In this case only thegetPosition
(andsetPosition
on the mutable classes) would be available from theTrackerHit
class (whileTrackerHitPlane
would have more getters/setters). Note that in the current implementation these have to be actual data member of the wrapped types.Some implementation details
ObjPtrT
typedef for the correspondingObj*
ConstT
typedef for the correspondingConst
classpodio::GenericWrapper
steers whether a non-constgetValue
template function is available, that also gives access to mutable user facing classes. If it is false, only the const version will be available, that will also give access to theConst
classes. This is necessary to not introduce a way to do an accidentalconst_cast
, by initializing the wrapper with aConst
value but then getting a mutable value out of it.interfaces
?)GenericWrapper
class (I really think the current name doesn't really capture the intent, but I haven't found a better one yet)