From c7c6f3b50b88e4294704f2d2c4a2e41cf9f91f2e Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 6 Oct 2014 18:30:04 +0200 Subject: [PATCH 1/4] Declare TriggerTimingReport as a struct not a class clang caught an inconsistency in declaring a struct as a class. --- FWCore/Framework/src/SystemTimeKeeper.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/FWCore/Framework/src/SystemTimeKeeper.h b/FWCore/Framework/src/SystemTimeKeeper.h index 0d8a2ebbfe2a7..d67cd94b7bccd 100644 --- a/FWCore/Framework/src/SystemTimeKeeper.h +++ b/FWCore/Framework/src/SystemTimeKeeper.h @@ -36,7 +36,7 @@ namespace edm { class PathContext; class HLTPathStatus; class ModuleCallingContext; - class TriggerTimingReport; + struct TriggerTimingReport; namespace service { class TriggersNameService; } From 7e5e5cf774e6ba0fd952183808415fdb4ee9e632 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 6 Oct 2014 18:32:48 +0200 Subject: [PATCH 2/4] Removed unused variables clang warned about existence of unused variables. These have been removed. --- FWCore/Integration/test/ThingAlgorithm.h | 3 +-- FWCore/Integration/test/TrackOfThingsProducer.cc | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/FWCore/Integration/test/ThingAlgorithm.h b/FWCore/Integration/test/ThingAlgorithm.h index 2a9f2a22bbea1..c5ccb272fe005 100644 --- a/FWCore/Integration/test/ThingAlgorithm.h +++ b/FWCore/Integration/test/ThingAlgorithm.h @@ -9,14 +9,13 @@ namespace edmtest { class ThingAlgorithm { public: - ThingAlgorithm(long iOffsetDelta = 0, int nThings = 20) : theDebugLevel(0), offset(0), offsetDelta(iOffsetDelta), + ThingAlgorithm(long iOffsetDelta = 0, int nThings = 20) : offset(0), offsetDelta(iOffsetDelta), nThings_(nThings) {} /// Runs the algorithm and returns a list of Things /// The user declares the vector and calls this method. void run(ThingCollection& thingCollection); private: - int theDebugLevel; long offset; long offsetDelta; int nThings_; diff --git a/FWCore/Integration/test/TrackOfThingsProducer.cc b/FWCore/Integration/test/TrackOfThingsProducer.cc index 3c80eb4aa7d1b..4e56ac5359054 100644 --- a/FWCore/Integration/test/TrackOfThingsProducer.cc +++ b/FWCore/Integration/test/TrackOfThingsProducer.cc @@ -62,7 +62,6 @@ namespace edmtest { // and have no meaning other than that we will later check that we can // read out what we put in here. std::vector::const_iterator key = keysToReference_.begin(); - std::vector::const_iterator keyEnd = keysToReference_.end(); for(unsigned int i = 0; i < 5; ++i) { From 4f961918eb70d042fbacf58099c1035fe1a117b5 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 6 Oct 2014 18:33:56 +0200 Subject: [PATCH 3/4] Hide the fact that the expression values are irrelevant for the unit test clang was complaining that the values generated by the expressions passed to CPPUNIT_ASSERT_THROW were never used. That is expected since they are supposed to thrown an exception. Slightly reworking the expression solved that problem. --- FWCore/FWLite/test/ref_t.cppunit.cpp | 42 +++++++++---------- .../Framework/test/iovsyncvalue_t.cppunit.cc | 10 ++--- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/FWCore/FWLite/test/ref_t.cppunit.cpp b/FWCore/FWLite/test/ref_t.cppunit.cpp index 238a38e3ed038..2ed39178638c0 100644 --- a/FWCore/FWLite/test/ref_t.cppunit.cpp +++ b/FWCore/FWLite/test/ref_t.cppunit.cpp @@ -388,29 +388,29 @@ void testRefInROOT::testThinning() { CPPUNIT_ASSERT(trackD.refVector1[0]->a == 10 + offset); CPPUNIT_ASSERT(trackD.refVector1[4]->a == 14 + offset); - CPPUNIT_ASSERT_THROW(trackD.refVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackD.refVector1[8].operator->(), cms::Exception); CPPUNIT_ASSERT(!trackD.refVector1.isAvailable()); CPPUNIT_ASSERT(trackD.refVector1[0]->a == 10 + offset); CPPUNIT_ASSERT(trackD.refVector1[4]->a == 14 + offset); - CPPUNIT_ASSERT_THROW(trackD.refVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackD.refVector1[8].operator->(), cms::Exception); CPPUNIT_ASSERT(trackD.ptrVector1[0]->a == 10 + offset); CPPUNIT_ASSERT(trackD.ptrVector1[4]->a == 14 + offset); - CPPUNIT_ASSERT_THROW(trackD.ptrVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackD.ptrVector1[8].operator->(), cms::Exception); CPPUNIT_ASSERT(trackD.ptrVector1[9]->a == 21 + offset); CPPUNIT_ASSERT(!trackD.ptrVector1.isAvailable()); CPPUNIT_ASSERT(trackD.ptrVector1[0]->a == 10 + offset); CPPUNIT_ASSERT(trackD.ptrVector1[4]->a == 14 + offset); - CPPUNIT_ASSERT_THROW(trackD.ptrVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackD.ptrVector1[8].operator->(), cms::Exception); CPPUNIT_ASSERT(trackD.ptrVector1[9]->a == 21 + offset); CPPUNIT_ASSERT(trackD.refToBaseVector1[0]->a == 10 + offset); CPPUNIT_ASSERT(trackD.refToBaseVector1[4]->a == 14 + offset); - CPPUNIT_ASSERT_THROW(trackD.refToBaseVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackD.refToBaseVector1[8].operator->(), cms::Exception); CPPUNIT_ASSERT(!trackD.refToBaseVector1.isAvailable()); CPPUNIT_ASSERT(trackD.refToBaseVector1[0]->a == 10 + offset); CPPUNIT_ASSERT(trackD.refToBaseVector1[4]->a == 14 + offset); - CPPUNIT_ASSERT_THROW(trackD.refToBaseVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackD.refToBaseVector1[8].operator->(), cms::Exception); // In the G branch this tests accessing a value in // thinned collection made from a master collection. @@ -462,11 +462,11 @@ void testRefInROOT::testThinning() { edmtest::TrackOfThings const& trackM0 = vTracks->at(0); CPPUNIT_ASSERT(!trackM0.ref1.isAvailable()); - CPPUNIT_ASSERT_THROW(trackM0.ref1->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM0.ref1.operator->(), cms::Exception); CPPUNIT_ASSERT(!trackM0.ptr1.isAvailable()); - CPPUNIT_ASSERT_THROW(trackM0.ptr1->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM0.ptr1.operator->(), cms::Exception); CPPUNIT_ASSERT(!trackM0.refToBase1.isAvailable()); - CPPUNIT_ASSERT_THROW(trackM0.refToBase1->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM0.refToBase1.operator->(), cms::Exception); edmtest::TrackOfThings const& trackM1 = vTracks->at(1); CPPUNIT_ASSERT(trackM1.ref1.isAvailable()); @@ -477,29 +477,29 @@ void testRefInROOT::testThinning() { CPPUNIT_ASSERT(trackM1.refToBase1->a == 44 + offset); edmtest::TrackOfThings const& trackM = vTracks->at(0); - CPPUNIT_ASSERT_THROW(trackM.refVector1[0]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.refVector1[0].operator->(), cms::Exception); CPPUNIT_ASSERT(trackM.refVector1[4]->a == 44 + offset); - CPPUNIT_ASSERT_THROW(trackM.refVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.refVector1[8].operator->(), cms::Exception); CPPUNIT_ASSERT(!trackM.refVector1.isAvailable()); - CPPUNIT_ASSERT_THROW(trackM.refVector1[0]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.refVector1[0].operator->(), cms::Exception); CPPUNIT_ASSERT(trackM.refVector1[4]->a == 44 + offset); - CPPUNIT_ASSERT_THROW(trackM.refVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.refVector1[8].operator->(), cms::Exception); - CPPUNIT_ASSERT_THROW(trackM.ptrVector1[0]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.ptrVector1[0].operator->(), cms::Exception); CPPUNIT_ASSERT(trackM.ptrVector1[4]->a == 44 + offset); - CPPUNIT_ASSERT_THROW(trackM.ptrVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.ptrVector1[8].operator->(), cms::Exception); CPPUNIT_ASSERT(!trackM.ptrVector1.isAvailable()); - CPPUNIT_ASSERT_THROW(trackM.ptrVector1[0]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.ptrVector1[0].operator->(), cms::Exception); CPPUNIT_ASSERT(trackM.ptrVector1[4]->a == 44 + offset); - CPPUNIT_ASSERT_THROW(trackM.ptrVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.ptrVector1[8].operator->(), cms::Exception); - CPPUNIT_ASSERT_THROW(trackM.refToBaseVector1[0]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.refToBaseVector1[0].operator->(), cms::Exception); CPPUNIT_ASSERT(trackM.refToBaseVector1[4]->a == 44 + offset); - CPPUNIT_ASSERT_THROW(trackM.refToBaseVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.refToBaseVector1[8].operator->(), cms::Exception); CPPUNIT_ASSERT(!trackM.refToBaseVector1.isAvailable()); - CPPUNIT_ASSERT_THROW(trackM.refToBaseVector1[0]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.refToBaseVector1[0].operator->(), cms::Exception); CPPUNIT_ASSERT(trackM.refToBaseVector1[4]->a == 44 + offset); - CPPUNIT_ASSERT_THROW(trackM.refToBaseVector1[8]->a, cms::Exception); + CPPUNIT_ASSERT_THROW(trackM.refToBaseVector1[8].operator->(), cms::Exception); } } diff --git a/FWCore/Framework/test/iovsyncvalue_t.cppunit.cc b/FWCore/Framework/test/iovsyncvalue_t.cppunit.cc index 6ede78a5ccc02..857fbb4fee537 100644 --- a/FWCore/Framework/test/iovsyncvalue_t.cppunit.cc +++ b/FWCore/Framework/test/iovsyncvalue_t.cppunit.cc @@ -174,10 +174,10 @@ testIOVSyncValue::invalidComparisonTest() CPPUNIT_ASSERT(! timeBased.comparable(eventBased)); CPPUNIT_ASSERT(! eventBased.comparable(timeBased)); - CPPUNIT_ASSERT_THROW( (timeBased < eventBased) , cms::Exception); - CPPUNIT_ASSERT_THROW( (timeBased <= eventBased) , cms::Exception); + CPPUNIT_ASSERT_THROW( [&](){return timeBased < eventBased;}() , cms::Exception); + CPPUNIT_ASSERT_THROW( [&](){return timeBased <= eventBased;}() , cms::Exception); CPPUNIT_ASSERT( !(timeBased == eventBased)); - CPPUNIT_ASSERT( (timeBased != eventBased)); - CPPUNIT_ASSERT_THROW( (timeBased > eventBased) , cms::Exception); - CPPUNIT_ASSERT_THROW( (timeBased >= eventBased) , cms::Exception); + CPPUNIT_ASSERT( timeBased != eventBased); + CPPUNIT_ASSERT_THROW( [&]() {return timeBased > eventBased;}() , cms::Exception); + CPPUNIT_ASSERT_THROW( [&]() {return timeBased >= eventBased;}() , cms::Exception); } From 276be4d660cbc2ab3ebdfbfcc39c50d79afe1c53 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 6 Oct 2014 19:50:43 +0200 Subject: [PATCH 4/4] Move code shared by executable and plugin to an existing library The unit test analyzer TestBeginEndJobAnalyzer has a static used by the unit test to see that all transitions happened. When compiled with clang the unit test failed presumably because we had mulitple copies of the static. This change fixes the problem and makes the code obey the C++ one definition rule. --- FWCore/Framework/test/BuildFile.xml | 6 ++-- .../test/stubs/TestBeginEndJobAnalyzer.cc | 6 ++++ .../test/stubs/TestBeginEndJobAnalyzer.h | 35 +++++++++---------- 3 files changed, 26 insertions(+), 21 deletions(-) diff --git a/FWCore/Framework/test/BuildFile.xml b/FWCore/Framework/test/BuildFile.xml index 39ad996d6c445..b5d260042c411 100644 --- a/FWCore/Framework/test/BuildFile.xml +++ b/FWCore/Framework/test/BuildFile.xml @@ -2,7 +2,7 @@ - + @@ -131,8 +131,9 @@ - + + @@ -169,6 +170,7 @@ + diff --git a/FWCore/Framework/test/stubs/TestBeginEndJobAnalyzer.cc b/FWCore/Framework/test/stubs/TestBeginEndJobAnalyzer.cc index bc46a2880c587..511a023c85506 100644 --- a/FWCore/Framework/test/stubs/TestBeginEndJobAnalyzer.cc +++ b/FWCore/Framework/test/stubs/TestBeginEndJobAnalyzer.cc @@ -26,6 +26,12 @@ void hello(const char * hi) { //std::cout << "IN " << hi << std::endl; } +TestBeginEndJobAnalyzer::Control & +TestBeginEndJobAnalyzer::control() { + static Control l; + return l; +} + TestBeginEndJobAnalyzer::TestBeginEndJobAnalyzer(const edm::ParameterSet& /* iConfig */) { hello("constr"); } diff --git a/FWCore/Framework/test/stubs/TestBeginEndJobAnalyzer.h b/FWCore/Framework/test/stubs/TestBeginEndJobAnalyzer.h index 7c700a9464f7d..0e13fb66e718c 100644 --- a/FWCore/Framework/test/stubs/TestBeginEndJobAnalyzer.h +++ b/FWCore/Framework/test/stubs/TestBeginEndJobAnalyzer.h @@ -30,31 +30,28 @@ class TestBeginEndJobAnalyzer : public edm::EDAnalyzer { ~TestBeginEndJobAnalyzer(); - virtual void analyze(const edm::Event&, const edm::EventSetup&); + virtual void analyze(const edm::Event&, const edm::EventSetup&) override; - virtual void beginJob(); - virtual void endJob(); - virtual void beginRun(edm::Run const&, edm::EventSetup const&); - virtual void endRun(edm::Run const&, edm::EventSetup const&); - virtual void beginLuminosityBlock(edm::LuminosityBlock const&, edm::EventSetup const&); - virtual void endLuminosityBlock(edm::LuminosityBlock const&, edm::EventSetup const&); + virtual void beginJob() override; + virtual void endJob() override; + virtual void beginRun(edm::Run const&, edm::EventSetup const&) override; + virtual void endRun(edm::Run const&, edm::EventSetup const&) override; + virtual void beginLuminosityBlock(edm::LuminosityBlock const&, edm::EventSetup const&) override; + virtual void endLuminosityBlock(edm::LuminosityBlock const&, edm::EventSetup const&) override; - struct dso_export Control { - bool beginJobCalled; - bool endJobCalled; - bool beginRunCalled; - bool endRunCalled; - bool beginLumiCalled; - bool endLumiCalled; - bool destructorCalled; + struct Control { + bool beginJobCalled = false; + bool endJobCalled = false; + bool beginRunCalled = false; + bool endRunCalled = false; + bool beginLumiCalled = false; + bool endLumiCalled = false; + bool destructorCalled = false; }; - static Control & control() dso_export { - static Control l; - return l; - } + static Control & control(); private: