Skip to content
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

DQM: header and namespace cleanup #27476

Merged
merged 27 commits into from Jul 22, 2019

Conversation

schneiml
Copy link
Contributor

@schneiml schneiml commented Jul 10, 2019

PR description:

This PR separates a few changes out of the big DQMStore restructuring project [1] that can be merged safely. No behaviour changes. These changes include things that need to be fixed/explicitly declared in subsystem code, therefore this PR touches many packages and will conflict almost certainly with others.

This is the reason to split out these changes and merge them now.

There are two big sets of changes:

  • Simplify includes. The only files to be included from the DQMServices/Core package are DQMStore.h and the module base classes (DQMEDAnalyzer.h, DQMEDHarvester.h, oneDQMEDAnalyzer.h, DQMGlobalEDAnalyzer.h). All others should the replaced with DQMStore.h.

    • This implies that MonitorElement.h should never be included, the majority the majority of changes is removing these.
    • There are still a lot of redundant includes and a few non-standard ones, that I did not bother to hunt down.
    • DQMStore and MonitorElement should never be forward-declared, to prevent problems in the next steps.
  • Namespace MonitorElement and DQMStore into dqm::reco::, dqm::harvesting:: and dqm::legacy:: (and dqm::impl::, which is the actual implementation).

    • There is (intentionally) now more DQMStore and MonitorElement in the default namespace. Most code will continue to work unchanged since DQMEDAnalyzer and DQMEDHarvester import them into class scope.
    • For now all of these are the same class, but in the future they will start to expose different APIs.
    • All legacy modules, that is those using DQM but not deriving from DQMEDAnalyzer or DQMEDHarvester, need to import the correct definition themselves. typedefs to to that are the second big batch of changes.
    • Many of the classes that now import dqm::legacy:: are actually helper classes and not legacy modules by themselves. These could be changed to dqm::reco:: or dqm::harvesting:: based on the APIs that they use, but that will be easier once the reduced APIs are actually in place.
    • The only visible change in non-legacy code is that functions returning MonitorElement defined outside the class declaration now need to qualify the MonitorElement in their return type. This is due to how C++ scoping works and hard to avoid.

In addition,

  • A lot of DQMServices/Examples is deleted, since it demonstrates the usage of legacy APIs, some of which are only used in the example code...
  • MonitorElement::Kind got turned into an enum class to keep things a bit more clean in the future.

PR validation:

Code compiles.

None of the changes should affect the output, but bugs and mistakes are possible, so we need to watch the comparisons.

PR comparison shows no significant changes.

[1] master...schneiml:dqm-new-dqmstore-on-CMSSW_11_0_0_pre1-from-andrius

Is this a scram bug? The test/ BuildFile does add DQMServices/Core already.
The error was:
DQMServices/Core/interface/DQMStore.h:23:10: fatal error: 'classlib/utils/Regexp.h' file not found
#include <classlib/utils/Regexp.h>
... where the tool was not smart enough.
All removing a newline that one of the header cleanup tools left over.
... so we can merge the subsystem changes before the big  restructuring.

Requires a bunch of changes to the ancient code in this package.
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27476/10805

@@ -14,6 +14,7 @@
*/

#include "FastSimulation/MaterialEffects/interface/MaterialEffectsSimulator.h"
#include "DQMServices/Core/interface/DQMStore.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schneiml @ssekmen @sbein as ME are commented, is this (and the previous update to BuildFile) really needed? Otherwise it would be good to clean it (possibly in a separate PR)

@fabiocos
Copy link
Contributor

@qliphy @efeyazgan @alberto-sanchez please check also the update in your domain, it looks straightforward

@pohsun
Copy link

pohsun commented Jul 19, 2019

+1

@alberto-sanchez
Copy link
Member

+1

indeed the changes are associated to RivetInterface and are straightforward

@ggovi
Copy link
Contributor

ggovi commented Jul 21, 2019

+1

@fabiocos
Copy link
Contributor

@schneiml I assume this is ok for you, please sign it, I would like to merge it tomorrow

@civanch @sbein @ssekmen @rekovic @peruzzim the updates in your are look ok to me, please check them anyway

@ssekmen
Copy link
Contributor

ssekmen commented Jul 22, 2019

+1

@fabiocos
Copy link
Contributor

@schneiml @andrius-k @jfernan2 ping

@jfernan2
Copy link
Contributor

+1
@fabiocos Marcel is on holiday this week, I believe he was OK with merging it, we clearly forgot to discuss this on last Friday meeting, I cannot comment on the PR notes made though

@civanch
Copy link
Contributor

civanch commented Jul 22, 2019

+1

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment