-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Refactored code in StorageFactory #35435
Conversation
Changing inheritance to containment avoids diamond inheritance with Storage. Removed some unused methods.
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35435/25568
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages:
@makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
-1 Failed Tests: Build HeaderConsistency BuildI found compilation error when building: >> Package IORawData/SiPixelInputSources built Entering library rule at IORawData/SiPixelInputSources >> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/src/SealModule.cc >> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/src/PixelSLinkDataInputSource.cc In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/src/PixelSLinkDataInputSource.cc:24: /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/interface/PixelSLinkDataInputSource.h:52:19: error: 'Storage' was not declared in this scope; did you mean 'edm::storage::Storage'? 52 | std::unique_ptr storage; | ^~~~~~~ | edm::storage::Storage In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/interface/PixelSLinkDataInputSource.h:33, from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/src/PixelSLinkDataInputSource.cc:24: |
@makortel @dan131riley An additional posssible refactoring would be removing the interfaces IOInput, IOOutput and IOChannel all together as nothing in our system actually wants to talk just to those interfaces. I think this is all left over from the SEAL import. Thoughts on keeping or removing said interfaces? |
Here is a system diagram for the storage code |
@Dr15Jones I would definitely get rid of IOChannel, it's not a useful abstraction for the way we use StorageFactory, and its effect on the class hierarchy is pernicious. Getting rid of the IOInput and IOOutput sounds ok, but I don't feel nearly as strongly about them. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35435/25605
|
Pull request #35435 was updated. @SiewYan, @smuzaffar, @Dr15Jones, @mkirsano, @malbouis, @makortel, @Saptaparna, @cmsbuild, @GurpreetSinghChahal, @yuanchao, @agrohsje, @alberto-sanchez, @francescobrivio, @tvami can you please check and sign again. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9c5581/19211/summary.html Comparison SummarySummary:
|
@@ -47,6 +47,7 @@ namespace lhef { | |||
class LHEReader::FileSource : public LHEReader::Source { | |||
public: | |||
FileSource(const std::string &fileURL) { | |||
using namespace edm::storage; | |||
auto storage = StorageFactory::get()->open(fileURL, IOFlags::OpenRead); |
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.
Hi @Dr15Jones : Shouldn't it be edm::Storage::StorageFactory in this line according to our coding rules?
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've been unable to determine which coding rule to which you refer. Could you point me to the reference?
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 thought I got that comment once but indeed I cannot spot in the rules. Let me sign.
+generators |
@agrohsje our coding rules say not to use |
+alca
|
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
PR validation:
Code compiles.
Framework unit tests all pass.