-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Move CPUServiceBase
, RootHandlers
, and TimingServiceBase
to FWCore/AbstractServices
#47466
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47466/43898
|
A new Pull Request was created by @makortel for master. It involves the following packages:
@Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild, please test |
@Dr15Jones Could you review? |
+1 Size: This PR adds an extra 96KB to repository Comparison SummarySummary:
|
Comparison differences are related to #47071 |
#ifndef FWCore_Utilities_CPUServiceBase_h | ||
#define FWCore_Utilities_CPUServiceBase_h | ||
#ifndef FWCore_AbstractServices_interface_CPUServiceBase_h | ||
#define FWCore_AbstractServices_interface_CPUServiceBase_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.
We do not normally put _interface_
here. Should this be consistent or do you want to change all the other files?
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.
We have already some headers with _interface_
in FWCore. 4.1 in https://cms-sw.github.io/cms_coding_rules.html#4--technical-coding-rules-1 has the wording
If necessary to create a unique name, one can add the directory name
Strictly speaking the _interface
is not necessary here, as there are no other headers with the same file name in this package.
I think the question of including _interface_
or not when not strictly necessary is somewhat of a personal taste, and I'd be fine with us not being fully consistent with that.
+core |
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. @antoniovilela, @sextonkennedy, @mandrenguyen, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Following #47280 (where the package was created), motivated by #47175 (review). These three service base classes were "easy to move" from FWCore/Utilities. I envision two more PRs, one for
RandomNumberGenerator
and one forJobReport
.Resolves cms-sw/framework-team#1265
PR validation:
Code compiles