-
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
propagate the enableMT flag from DQMStore to MEtoEDMConverter and DQMRoo... #4745
Conversation
…RootOutputModule and make the DQMRootSource a friend with DQMStore
A new Pull Request was created by @deguio for CMSSW_7_2_X. propagate the enableMT flag from DQMStore to MEtoEDMConverter and DQMRoo... It involves the following packages: DQMServices/Components @ojeda, @danduggan, @rovere, @cmsbuild, @nclopezo, @deguio, @Degano can you please review it and eventually sign? Thanks. |
@@ -46,7 +46,6 @@ MEtoEDMConverter::MEtoEDMConverter(const edm::ParameterSet & iPSet) : | |||
// get dqm info | |||
dbe = 0; | |||
dbe = edm::Service<DQMStore>().operator->(); | |||
enableMultiThread_ = dbe->enableMultiThread_; |
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.
Could you also hide DQMStore::enableMultiThread_ behind a get function since public member data is not allowed 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.
hello chris,
the member is accessible only by the friend classes. it is not public
https://github.com/deguio/cmssw/blob/propagateEnableMT/DQMServices/Core/interface/DQMStore.h#L691
still not allowed? why?
thanks,
F.
@@ -714,6 +714,7 @@ class DQMStore | |||
friend class DQMArchiver; | |||
friend class DQMStoreExample; // for get{All,Matching}Contents -- sole user of this method! | |||
friend class DQMRootOutputModule; | |||
friend class DQMRootSource; |
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.
If you added a public function
bool multiThreadingEnabled() const { return enableMultiThread_; }
Then you wouldn't have to make DQMRootSource a friend. That would minimize coupling and make maintenance easier.
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.
Ok, it makes sense and I agree. But basically all the classes which are friend with DQM store will have to stay so because they access methods which will be made private soon as soon as the transition to dqmedanalyzer finishes.
-1 runTheMatrix-results/101.0_SingleElectronE120EHCAL+SingleElectronE120EHCAL/step1_SingleElectronE120EHCAL+SingleElectronE120EHCAL.log you can see the results of the tests here: |
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (but tests are reportedly failing). |
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine). |
DQMServices -- propagate the enableMT flag from DQMStore to MEtoEDMConverter and DQMRoo...
...tOutputModule and make the DQMRootSource a friend with DQMStore