Mark legacy EDModule constructors deprecated #37592
Conversation
Attributing the (base) class deprecated does not lead to as wide coverage of the deprecation warnings as we hoped, but attributing functions seems to work. Let's try marking the constructors as deprecated (on a small test it seems to work).
makortel
changed the title
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37592/29352
|
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages:
@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cfc78f/23964/summary.html Comparison SummarySummary:
|
|
I checked to build log, and
I checked a few of the files flagged by class annotation only, and in those the EDModule itself was already threading-aware, but the file |
OK. Although it really is the class that is deprecated (so what is there is actually better documentation) and we want to get rid of those headers so we do want to catch cases where the header is improperly included. |
I agree annotating the class is much more clear, but I was thinking if two different kinds of warnings for the same issue would be confusing. Or should we understand them as strictly speaking separate issues ( |
|
I've now gone through the 92 files flagged by the class annotation only (see PRs linking to this PR). There were 2 or 3 cases where the class annotation lead to a I'll next remove the class annotations and add explanatory comments. |
While in principle annotating the entire class would be the proper thing to do, with gcc the constructor annotation appears to work better for identifying the deriving classes that need to be migrated to thread-friendly base classes.
|
@cmsbuild, please test All differences are in workflow 9.0, let's try again to see if these are repeatable |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cfc78f/24315/summary.html Comparison SummarySummary:
|
|
@cmsbuild, please test 9.0 again |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cfc78f/24322/summary.html Comparison SummarySummary:
|
|
9.0 shows still differences. Maybe this is another symptom of #34448? The changes in this PR are purely technical. |
|
+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) |
cmsbuild
added
core-approved
fully-signed
and removed
core-pending
pending-signatures
labels
|
+1 |
PR description:
We've noticed on a few occasions that marking the legacy EDModule base classes as
CMS_DEPRECATEDdoes not cover all the classes that inherit from these base classes. Marking functions deprecated seems to be more reliable to generate warnings, so this PR marks the constructors of the legacy EDModule base classes asCMS_DEPRECATED.PR validation:
A legacy EDAnalyzer https://github.com/cms-sw/cmssw/blob/12_4_0_pre2/OnlineDB/SiStripO2O/plugins/SiStripPayloadMapTableCreator.cc that doesn't generate a warning in 12_4_0_pre2, generates a warning with this PR.