-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Implement a GenericCloner test module based on an edmplugin::PluginFactory
#49152
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
Implement a GenericCloner test module based on an edmplugin::PluginFactory
#49152
Conversation
|
cms-bot internal usage |
|
A new Pull Request was created by @ghyls for master. It involves the following packages:
The following packages do not have a category, yet: TrivialSerialisation/Common @Dr15Jones, @cmsbuild, @fwyzard, @makortel, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
fwyzard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of review.
| <!-- <use name="CondFormats/CSCObjects"/> --> | ||
| <!-- <use name="CondFormats/DataRecord"/> --> | ||
| <!-- <use name="DataFormats/MuonDetId"/> --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the commented-out lines
| #include "DataFormats/Common/interface/Wrapper.h" | ||
| #include "TrivialSerialisation/Common/interface/TrivialSerialiserBase.h" | ||
| #include "TrivialSerialisation/Common/interface/TrivialCopyTraits.h" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add explicit includes for all the types being used, like edm::AnyBuffer, edm::Exception, etc ?
|
|
||
| } // namespace ngt | ||
|
|
||
| #endif // TrivialSerialisation_Common_TrivialSerialiserSource_h No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing end-of-line at the end of the file.
| namespace ngt { | ||
| class TrivialSerialiserSourceBase { | ||
| public: | ||
| TrivialSerialiserSourceBase() {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| TrivialSerialiserSourceBase() {}; | |
| TrivialSerialiserSourceBase() = default; |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you, it should be = default
| #include "DataFormats/Common/interface/FillViewHelperVector.h" | ||
| #include "DataFormats/Provenance/interface/ViewTypeChecker.h" | ||
|
|
||
| #include <span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed ?
|
|
||
| ----------------------------------------------------------------------*/ | ||
|
|
||
| #include "DataFormats/Common/interface/AnyBuffer.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed ?
| #include <algorithm> | ||
| #include <cassert> | ||
| #include <memory> | ||
| #include <span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed ?
| #include "DataFormats/Common/interface/WrapperDetail.h" | ||
| #include "DataFormats/Provenance/interface/ProductID.h" | ||
| #include "FWCore/Utilities/interface/EDMException.h" | ||
| #include "FWCore/Utilities/interface/TypeDemangler.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file should go under DataFormats/Common ?
The interface described by the trait can also be used independently from the serialisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some unit tests for the traits ?
For example, check that some types (e.g. int or std::vector<float>) are properly supported, that some other types (e.g. std::map) are not supported, and that you can write a simple type and specialise the traits for it.
|
|
||
| namespace ngt { | ||
| template <typename T> | ||
| class TrivialSerialiserSource : public TrivialSerialiserSourceBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to find a name that does not contain Source, because Source has a specific meaning in CMSSW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, didn't think about that... TrivialSerialiserFactory and TrivialSerialiserBase are already taken, so maybe TrivialSerialiserGenerator could be a better option... Anyway, as you noticed the naming scheme is a bit complicated now:
I would suggest renaming:
TrivialSerialiserSourceFactory -> SerialiserFactory
TrivialSerialiserSourceBase -> SerialiserBase
TrivialSerialiserSource -> Serialiser
and keep TrivialSerialiser and TrivialSerialiserBase as they are.
So we would get Serialiser plugins from the factory, and use them to initialise const / mutable (readers / writers) TrivialSerialiser objects.
|
Can you add an example of how one will use this with SoA types ? |
923af7a to
ff045af
Compare
ff045af to
a54d2bb
Compare
|
Pull request #49152 was updated. |
Co-authored-by: Andrea Bocci <andrea.bocci@cern.ch>
Co-authored-by: Andrea Bocci <andrea.bocci@cern.ch>
…tMulticollection, and PortableHostObjects Co-authored-by: Andrea Bocci <andrea.bocci@cern.ch>
Co-authored-by: Andrea Bocci <andrea.bocci@cern.ch>
b0ef6bd to
94d5ad8
Compare
|
Pull request #49152 was updated. |
|
Milestone for this pull request has been moved to CMSSW_16_1_X. Please open a backport if it should also go in to CMSSW_16_0_X. |
|
Did #49475 supersede this PR? |
PR description:
Implements an alternative to #47504 based on a plugin factory. This PR and #47504 provide the same functionality:
The plugin factory returns
TrivialSerialiserSourceobjects for a given type.TrivialSerialisers can then be initialized by callingTrivialSerialiserSource::initializewith aWrapperBaseobject. The initialized TrivialSerialiser isconstor nonconstdepending on the constness of theWrapperBaseobject used to initialize it. A pointer to thisWrapperBaseobject is stored inTrivialSerialiserBaseasconst edm::WrapperBase*.PR validation:
Various unit tests have been included:
FWCore/TestModules/test/testGenericCloner_cfg.py: produce, clone and validate products of typeint,std::string, andedm::EventID. does not clone a transientcms.int32().DataFormats/Portable/test/testGenericCloner_cfg.py: Produce, clone and validate a portable object, a portable collection, and some portable multicollections.DataFormats/Common/test/test_catch2_TrivialCopyTraits.cpp: Implements aTrivialCopyTraitsspecialization for a simple struct. Tests the methods ofTrivialCopyTraitswith the typesint,double,std::vector<float>, and the struct.If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for: