Migrate from boost::optional to std::optional in EventSetup system #24586

knoepfel · 2018-09-18T17:20:25Z

This PR does include some whitespace changes which are due to using C++17's nested namespace declarations.

Includes whitespace changes due to using nested namespace declarations, which are supported as of C++17.

cmsbuild · 2018-09-18T17:21:42Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-09-18T17:25:29Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24586/6472

cmsbuild · 2018-09-18T17:25:52Z

A new Pull Request was created by @knoepfel (Kyle Knoepfel) for master.

It involves the following packages:

DQM/DTMonitorClient
DQM/EcalPreshowerMonitorModule
DQM/HcalCommon
DQM/RPCMonitorClient
DQM/SiPixelMonitorClient
DQM/SiStripMonitorClient
DQM/SiStripMonitorDigi
DQM/TrackingMonitorClient
DQMServices/Components
FWCore/Framework
FWCore/Modules
L1TriggerConfig/RCTConfigProducers
PhysicsTools/CondLiteIO

@smuzaffar, @andrius-k, @Dr15Jones, @kmaeshima, @schneiml, @monttj, @nsmith-, @rekovic, @cmsbuild, @jfernan2, @thomreis, @gpetruc, @arizzi can you please review it and eventually sign? Thanks.
@acimmino, @barvic, @DryRun, @hdelanno, @makortel, @mtosi, @wddgit, @argiro, @Martin-Grunewald, @fioriNTU, @deguio, @jandrea, @idebruyn, @threus, @venturia this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

andrius-k · 2018-09-18T17:53:11Z

please test

cmsbuild · 2018-09-18T17:53:33Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30491/console Started: 2018/09/18 19:54

Dr15Jones · 2018-09-18T18:34:04Z

DQMServices/Components/src/DQMDaqInfo.cc

@@ -11,7 +11,7 @@ void DQMDaqInfo::beginLuminosityBlock(const edm::LuminosityBlock& lumiBlock, con

  edm::eventsetup::EventSetupRecordKey recordKey(edm::eventsetup::EventSetupRecordKey::TypeTag::findType("RunInfoRcd"));

-  if( nullptr != iSetup.find( recordKey ) ) {
+  if( std::nullopt != iSetup.find( recordKey ) ) {


Should this also be changed to use tryToGet?

Yes...I missed that one. Will change.

Dr15Jones · 2018-09-18T18:42:29Z

FWCore/Framework/src/ESProducer.cc

@@ -2,7 +2,7 @@
 //
 // Package:     Framework
 // Class  :     ESProducer
-// 
+//


It looks like you only changed spaces here? Can this be reverted back?

Yikes, yes that's true. Will revert.

Dr15Jones · 2018-09-18T18:42:57Z

FWCore/Framework/interface/produce_helpers.h

-                                                            ToT & iTo) {
-       iTo = std::move(iFrom);
-     }
+namespace edm::eventsetup {


Was this just a change from 3 spaces to 2 ? If so, please revert.

Namespace collapsing was the change.

Dr15Jones · 2018-09-18T18:43:25Z

FWCore/Framework/interface/es_Label.h

+
+  struct Label {
+    Label() = default;
+    Label(std::string_view const iString) : default_{iString} {}


Was string_view the only change?

That and the namespace collapsing.

I think the switch to std::string_view is causing the compilation problem in HcalHardcodeGeometryEP.

Yes, just confirmed that. Will revert.

Dr15Jones · 2018-09-18T18:45:48Z

FWCore/Framework/test/DummyProxyProvider.h

@@ -4,7 +4,7 @@
 //
 // Package:     Framework
 // Class  :     DummyProvider
-// 
+//


Were only cosmetic changes done to this file?

Dr15Jones · 2018-09-18T18:46:08Z

FWCore/Framework/test/DummyData.h

-         };
-      }
-   }
+namespace edm::eventsetup::test {


Is this only cosmetic changes?

Dr15Jones · 2018-09-18T18:48:55Z

FWCore/Framework/interface/ESProducts.h

@@ -28,114 +28,113 @@

 // forward declarations
 namespace edm {


Are the only changes the spacing and the collapsed namespace? If so, please revert.

Yes, will revert.

Dr15Jones · 2018-09-18T18:49:31Z

FWCore/Framework/interface/ESProducer.h

@@ -4,7 +4,7 @@
 //
 // Package:     Framework
 // Class  :     ESProducer
-// 


Were there any functional changes to this file?

I think the answer is no. I'll be more careful next time.

knoepfel · 2018-09-18T19:17:03Z

L1TriggerConfig/RCTConfigProducers/src/L1RCTChannelMaskTester.cc

@@ -63,7 +63,7 @@ void L1RCTChannelMaskTester::analyze(const edm::Event& iEvent,
            edm::eventsetup::EventSetupRecordKey::TypeTag::findType(
                    "L1RCTNoisyChannelMaskRcd"));

-    if (evSetup.find(recordKey) == nullptr) {
+    if (evSetup.find(recordKey) == std::nullopt) {
        //record not found


Missed this one, too--should be tryToGet.

cmsbuild · 2018-09-18T20:03:17Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-09-19T14:26:55Z

Pull request #24586 was updated. @smuzaffar, @andrius-k, @Dr15Jones, @kmaeshima, @schneiml, @monttj, @nsmith-, @rekovic, @cmsbuild, @jfernan2, @thomreis, @ggovi, @gpetruc, @arizzi can you please check and sign again.

andrius-k · 2018-09-19T14:33:39Z

please test

cmsbuild · 2018-09-19T14:34:04Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30509/console Started: 2018/09/19 16:34

cmsbuild · 2018-09-19T20:41:36Z

+1
Tested at: 6374a14
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24586/30509/summary.html

cmsbuild · 2018-09-19T20:41:39Z

Comparison job queued.

cmsbuild · 2018-09-19T22:06:16Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24586/30509/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3146746
DQMHistoTests: Total failures: 77
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3146472
DQMHistoTests: Total skipped: 197
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 133 log files, 14 edm output root files, 32 DQM output files

andrius-k · 2018-09-21T13:29:30Z

+1

Dr15Jones · 2018-09-21T20:05:52Z

+1
The histogram failures are all from L1TEMU/L1TStage2CaloLayer2/L1TdeStage2CaloLayer2 which I believe has a history of false positives.

Dr15Jones · 2018-09-24T19:05:14Z

@monttj @arizzi @gpetruc @ggovi @rekovic @thomreis @nsmith-
please review and sign

thomreis · 2018-09-25T06:36:26Z

+1

ggovi · 2018-09-25T07:54:00Z

+1

fabiocos · 2018-09-25T12:09:56Z

+1

fabiocos · 2018-09-25T12:10:07Z

merge

knoepfel added 2 commits September 18, 2018 11:41

Switch from boost::optional to std::optional.

9444360

Modernizations.

53ea658

Includes whitespace changes due to using nested namespace declarations, which are supported as of C++17.

cmsbuild added this to the CMSSW_10_3_X milestone Sep 18, 2018

cmsbuild added analysis-pending code-checks-pending comparison-pending core-pending dqm-pending l1-pending orp-pending pending-signatures tests-pending labels Sep 18, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 18, 2018

cmsbuild added tests-started and removed tests-pending labels Sep 18, 2018

Dr15Jones suggested changes Sep 18, 2018

View reviewed changes

knoepfel commented Sep 18, 2018

View reviewed changes

knoepfel added 2 commits September 18, 2018 14:38

Switch to 'tryToGet' usage.

c194061

Revert stylistic changes.

593b48a

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-started labels Sep 18, 2018

cmsbuild added tests-started and removed tests-pending labels Sep 19, 2018

cmsbuild added tests-approved and removed tests-started labels Sep 19, 2018

cmsbuild added comparison-available and removed comparison-pending labels Sep 19, 2018

cmsbuild added dqm-approved and removed dqm-pending labels Sep 21, 2018

cmsbuild added core-approved and removed core-pending labels Sep 21, 2018

cmsbuild added l1-approved and removed l1-pending labels Sep 25, 2018

cmsbuild added db-approved and removed db-pending labels Sep 25, 2018

cmsbuild added orp-approved and removed orp-pending labels Sep 25, 2018

cmsbuild merged commit 5617275 into cms-sw:master Sep 25, 2018

Migrate from boost::optional to std::optional in EventSetup system #24586

Migrate from boost::optional to std::optional in EventSetup system #24586

Conversation

knoepfel commented Sep 18, 2018

cmsbuild commented Sep 18, 2018

cmsbuild commented Sep 18, 2018

cmsbuild commented Sep 18, 2018

andrius-k commented Sep 18, 2018

cmsbuild commented Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Sep 18, 2018

cmsbuild commented Sep 19, 2018

andrius-k commented Sep 19, 2018

cmsbuild commented Sep 19, 2018 • edited Loading

cmsbuild commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

andrius-k commented Sep 21, 2018

Dr15Jones commented Sep 21, 2018

Dr15Jones commented Sep 24, 2018

thomreis commented Sep 25, 2018

ggovi commented Sep 25, 2018

fabiocos commented Sep 25, 2018

fabiocos commented Sep 25, 2018

cmsbuild commented Sep 18, 2018 •

edited

Loading

cmsbuild commented Sep 19, 2018 •

edited

Loading