Merge pull request #28631 from Dr15Jones/mutableFWLite

Static analysis changes for DataFormats/FWLite
cms-sw · Dec 17, 2019 · a3d957f · a3d957f
2 parents 9df6b3e + 87e1876
commit a3d957f
Show file tree

Hide file tree

Showing 9 changed files with 80 additions and 58 deletions.
diff --git a/DataFormats/FWLite/interface/DataGetterHelper.h b/DataFormats/FWLite/interface/DataGetterHelper.h
@@ -24,6 +24,7 @@
 #include "DataFormats/FWLite/interface/InternalDataKey.h"
 #include "FWCore/FWLite/interface/BranchMapReader.h"
 #include "FWCore/Utilities/interface/propagate_const.h"
+#include "FWCore/Utilities/interface/thread_safety_macros.h"
 
 #include "Rtypes.h"
 
@@ -52,12 +53,13 @@ namespace fwlite {
   class DataGetterHelper {
   public:
     //            DataGetterHelper() {};
-    DataGetterHelper(TTree* tree,
-                     std::shared_ptr<HistoryGetterBase> historyGetter,
-                     std::shared_ptr<BranchMapReader> branchMap = std::shared_ptr<BranchMapReader>(),
-                     std::shared_ptr<edm::EDProductGetter> getter = std::shared_ptr<edm::EDProductGetter>(),
-                     bool useCache = false,
-                     std::function<void(TBranch const&)> baFunc = [](TBranch const&) {});
+    DataGetterHelper(
+        TTree* tree,
+        std::shared_ptr<HistoryGetterBase> historyGetter,
+        std::shared_ptr<BranchMapReader> branchMap = std::shared_ptr<BranchMapReader>(),
+        std::shared_ptr<edm::EDProductGetter> getter = std::shared_ptr<edm::EDProductGetter>(),
+        bool useCache = false,
+        std::function<void(TBranch const&)> baFunc = [](TBranch const&) {});
     virtual ~DataGetterHelper();
 
     // ---------- const member functions ---------------------
@@ -96,16 +98,18 @@ namespace fwlite {
 
     // ---------- member data --------------------------------
     TTree* tree_;
-    mutable std::shared_ptr<BranchMapReader> branchMap_;
-    mutable KeyToDataMap data_;
-    mutable std::vector<char const*> labels_;
+    //This class is not inteded to be used across different threads
+    CMS_SA_ALLOW mutable std::shared_ptr<BranchMapReader> branchMap_;
+    CMS_SA_ALLOW mutable KeyToDataMap data_;
+    CMS_SA_ALLOW mutable std::vector<char const*> labels_;
     const edm::ProcessHistory& history() const;
 
-    mutable std::map<std::pair<edm::ProductID, edm::BranchListIndex>, std::shared_ptr<internal::Data>> idToData_;
-    mutable std::map<edm::BranchID, std::shared_ptr<internal::Data>> bidToData_;
+    CMS_SA_ALLOW mutable std::map<std::pair<edm::ProductID, edm::BranchListIndex>, std::shared_ptr<internal::Data>>
+        idToData_;
+    CMS_SA_ALLOW mutable std::map<edm::BranchID, std::shared_ptr<internal::Data>> bidToData_;
     edm::propagate_const<std::shared_ptr<fwlite::HistoryGetterBase>> historyGetter_;
     std::shared_ptr<edm::EDProductGetter const> getter_;
-    mutable bool tcTrained_;
+    CMS_SA_ALLOW mutable bool tcTrained_;
     /// Use internal TTreeCache.
     const bool tcUse_;
     /// Branch-access-function gets called whenever a branch data is accessed.

diff --git a/DataFormats/FWLite/interface/Event.h b/DataFormats/FWLite/interface/Event.h
@@ -36,6 +36,7 @@
  }
  \endcode
 
+ NOTE: This class is not safe to use across threads.
 */
 //
 // Original Author:  Chris Jones
@@ -62,6 +63,7 @@
 #include "DataFormats/Provenance/interface/EventProcessHistoryID.h"
 #include "DataFormats/Provenance/interface/EventAuxiliary.h"
 #include "DataFormats/Provenance/interface/EventID.h"
+#include "FWCore/Utilities/interface/thread_safety_macros.h"
 
 // forward declarations
 namespace edm {
@@ -92,7 +94,8 @@ namespace fwlite {
     // DataGetterHelper caching is enabled. When user sets useCache to
     // false no cache is created unless user attaches and controls it
     // himself.
-    Event(TFile* iFile, bool useCache = true, std::function<void(TBranch const&)> baFunc = [](TBranch const&) {});
+    Event(
+        TFile* iFile, bool useCache = true, std::function<void(TBranch const&)> baFunc = [](TBranch const&) {});
     ~Event() override;
 
     ///Advance to next event in the TFile
@@ -192,29 +195,30 @@ namespace fwlite {
     void setGetter(std::shared_ptr<edm::EDProductGetter const> getter) { return dataHelper_.setGetter(getter); }
 
     // ---------- member data --------------------------------
-    mutable TFile* file_;
+    //This class is not inteded to be used across different threads
+    CMS_SA_ALLOW mutable TFile* file_;
     // TTree* eventTree_;
     TTree* eventHistoryTree_;
     // Long64_t eventIndex_;
-    mutable std::shared_ptr<fwlite::LuminosityBlock> lumi_;
-    mutable std::shared_ptr<fwlite::Run> run_;
-    mutable fwlite::BranchMapReader branchMap_;
+    CMS_SA_ALLOW mutable std::shared_ptr<fwlite::LuminosityBlock> lumi_;
+    CMS_SA_ALLOW mutable std::shared_ptr<fwlite::Run> run_;
+    CMS_SA_ALLOW mutable fwlite::BranchMapReader branchMap_;
 
     //takes ownership of the strings used by the DataKey keys in data_
-    mutable std::vector<char const*> labels_;
-    mutable edm::ProcessHistoryMap historyMap_;
-    mutable std::vector<edm::EventProcessHistoryID> eventProcessHistoryIDs_;
-    mutable std::vector<std::string> procHistoryNames_;
-    mutable edm::EventAuxiliary aux_;
-    mutable EntryFinder entryFinder_;
+    CMS_SA_ALLOW mutable std::vector<char const*> labels_;
+    CMS_SA_ALLOW mutable edm::ProcessHistoryMap historyMap_;
+    CMS_SA_ALLOW mutable std::vector<edm::EventProcessHistoryID> eventProcessHistoryIDs_;
+    CMS_SA_ALLOW mutable std::vector<std::string> procHistoryNames_;
+    CMS_SA_ALLOW mutable edm::EventAuxiliary aux_;
+    CMS_SA_ALLOW mutable EntryFinder entryFinder_;
     edm::EventAuxiliary const* pAux_;
     edm::EventAux const* pOldAux_;
     TBranch* auxBranch_;
     int fileVersion_;
-    mutable bool parameterSetRegistryFilled_;
+    CMS_SA_ALLOW mutable bool parameterSetRegistryFilled_;
 
     fwlite::DataGetterHelper dataHelper_;
-    mutable std::shared_ptr<RunFactory> runFactory_;
+    CMS_SA_ALLOW mutable std::shared_ptr<RunFactory> runFactory_;
   };
 
 }  // namespace fwlite

diff --git a/DataFormats/FWLite/interface/EventSetup.h b/DataFormats/FWLite/interface/EventSetup.h
@@ -37,6 +37,7 @@
       std::cout << fooHandle->value()<<std::endl;
     }
 
+    NOTE: This class is not safe to use across threads
 */
 //
 // Original Author:
@@ -51,6 +52,7 @@
 #include "DataFormats/Provenance/interface/EventID.h"
 #include "DataFormats/Provenance/interface/Timestamp.h"
 #include "FWCore/Utilities/interface/propagate_const.h"
+#include "FWCore/Utilities/interface/thread_safety_macros.h"
 
 // forward declarations
 class TFile;
@@ -103,9 +105,10 @@ namespace fwlite {
     edm::EventID m_syncedEvent;
     edm::Timestamp m_syncedTimestamp;
 
-    mutable TFile* m_file;
+    //This class is not inteded to be used across different threads
+    CMS_SA_ALLOW mutable TFile* m_file;
 
-    mutable std::vector<Record*> m_records;
+    CMS_SA_ALLOW mutable std::vector<Record*> m_records;
   };
 }  // namespace fwlite
 

diff --git a/DataFormats/FWLite/interface/LuminosityBlock.h b/DataFormats/FWLite/interface/LuminosityBlock.h
@@ -10,7 +10,7 @@
    Description: <one line class summary>
 
    Usage:
-   <usage>
+   This class is not safe to use across threads
 
 */
 //
@@ -33,6 +33,7 @@
 #include "DataFormats/FWLite/interface/EntryFinder.h"
 #include "DataFormats/Provenance/interface/ProcessHistoryRegistry.h"
 #include "DataFormats/Provenance/interface/LuminosityBlockAuxiliary.h"
+#include "FWCore/Utilities/interface/thread_safety_macros.h"
 
 // forward declarations
 namespace edm {
@@ -111,23 +112,24 @@ namespace fwlite {
     void updateAux(Long_t lumiIndex) const;
 
     // ---------- member data --------------------------------
-    mutable std::shared_ptr<BranchMapReader> branchMap_;
+    //This class is not inteded to be used across different threads
+    CMS_SA_ALLOW mutable std::shared_ptr<BranchMapReader> branchMap_;
 
-    mutable std::shared_ptr<fwlite::Run> run_;
+    CMS_SA_ALLOW mutable std::shared_ptr<fwlite::Run> run_;
 
     //takes ownership of the strings used by the DataKey keys in data_
-    mutable std::vector<char const*> labels_;
-    mutable edm::ProcessHistoryMap historyMap_;
-    mutable std::vector<std::string> procHistoryNames_;
-    mutable edm::LuminosityBlockAuxiliary aux_;
-    mutable EntryFinder entryFinder_;
+    CMS_SA_ALLOW mutable std::vector<char const*> labels_;
+    CMS_SA_ALLOW mutable edm::ProcessHistoryMap historyMap_;
+    CMS_SA_ALLOW mutable std::vector<std::string> procHistoryNames_;
+    CMS_SA_ALLOW mutable edm::LuminosityBlockAuxiliary aux_;
+    CMS_SA_ALLOW mutable EntryFinder entryFinder_;
     edm::LuminosityBlockAuxiliary const* pAux_;
     edm::LuminosityBlockAux const* pOldAux_;
     TBranch* auxBranch_;
     int fileVersion_;
 
     DataGetterHelper dataHelper_;
-    mutable std::shared_ptr<RunFactory> runFactory_;
+    CMS_SA_ALLOW mutable std::shared_ptr<RunFactory> runFactory_;
   };
 
 }  // namespace fwlite

diff --git a/DataFormats/FWLite/interface/Record.h b/DataFormats/FWLite/interface/Record.h
@@ -28,6 +28,7 @@
 // user include files
 #include "DataFormats/FWLite/interface/IOVSyncValue.h"
 #include "FWCore/Utilities/interface/TypeID.h"
+#include "FWCore/Utilities/interface/thread_safety_macros.h"
 
 // forward declarations
 class TTree;
@@ -78,7 +79,8 @@ namespace fwlite {
     IOVSyncValue m_start;
     IOVSyncValue m_end;
 
-    mutable std::map<std::pair<edm::TypeID, std::string>, std::pair<TBranch*, void*>> m_branches;
+    //This class is not inteded to be used across different threads
+    CMS_SA_ALLOW mutable std::map<std::pair<edm::TypeID, std::string>, std::pair<TBranch*, void*>> m_branches;
   };
 
   template <typename HANDLE>

diff --git a/DataFormats/FWLite/interface/Run.h b/DataFormats/FWLite/interface/Run.h
@@ -10,7 +10,7 @@
    Description: <one line class summary>
 
    Usage:
-   <usage>
+   This class is not safe to use across different threads
 
 */
 //
@@ -35,6 +35,7 @@
 #include "DataFormats/Provenance/interface/RunID.h"
 #include "FWCore/FWLite/interface/BranchMapReader.h"
 #include "DataFormats/FWLite/interface/DataGetterHelper.h"
+#include "FWCore/Utilities/interface/thread_safety_macros.h"
 
 // forward declarations
 namespace edm {
@@ -107,14 +108,15 @@ namespace fwlite {
     void updateAux(Long_t runIndex) const;
 
     // ---------- member data --------------------------------
-    mutable std::shared_ptr<BranchMapReader> branchMap_;
+    //This class is not inteded to be used across different threads
+    CMS_SA_ALLOW mutable std::shared_ptr<BranchMapReader> branchMap_;
 
     //takes ownership of the strings used by the DataKey keys in data_
-    mutable std::vector<char const*> labels_;
-    mutable edm::ProcessHistoryMap historyMap_;
-    mutable std::vector<std::string> procHistoryNames_;
-    mutable edm::RunAuxiliary aux_;
-    mutable EntryFinder entryFinder_;
+    CMS_SA_ALLOW mutable std::vector<char const*> labels_;
+    CMS_SA_ALLOW mutable edm::ProcessHistoryMap historyMap_;
+    CMS_SA_ALLOW mutable std::vector<std::string> procHistoryNames_;
+    CMS_SA_ALLOW mutable edm::RunAuxiliary aux_;
+    CMS_SA_ALLOW mutable EntryFinder entryFinder_;
     edm::RunAuxiliary const* pAux_;
     edm::RunAux const* pOldAux_;
     TBranch* auxBranch_;

diff --git a/DataFormats/FWLite/interface/RunFactory.h b/DataFormats/FWLite/interface/RunFactory.h
@@ -21,6 +21,7 @@
 #include <memory>
 
 #include "DataFormats/FWLite/interface/Run.h"
+#include "FWCore/Utilities/interface/thread_safety_macros.h"
 
 namespace fwlite {
   class RunFactory {
@@ -35,7 +36,8 @@ namespace fwlite {
     RunFactory(const RunFactory&) = delete;  // stop default
 
     const RunFactory& operator=(const RunFactory&) = delete;  // stop default
-    mutable std::shared_ptr<fwlite::Run> run_;
+    //This class is not inteded to be used across different threads
+    CMS_SA_ALLOW mutable std::shared_ptr<fwlite::Run> run_;
 
     // ---------- member data --------------------------------
   };

diff --git a/DataFormats/FWLite/src/DataGetterHelper.cc b/DataFormats/FWLite/src/DataGetterHelper.cc
@@ -43,8 +43,8 @@ namespace fwlite {
   // static data member definitions
   //
   // empty object used to signal that the branch requested was not found
-  static internal::Data branchNotFound;
-  static char kEmptyString[1] = {0};
+  static internal::Data branchNotFound{};
+  static const char kEmptyString[1] = {0};
 
   //
   // constructors and destructor
@@ -208,18 +208,20 @@ namespace fwlite {
       std::strncpy(newModule, iModuleLabel, moduleLabelLen);
       labels_.push_back(newModule);
 
-      char* newProduct = kEmptyString;
+      char const* newProduct = kEmptyString;
       if (key.product()[0] != 0) {
         size_t newProductLen = strlen(key.product()) + 1;
-        newProduct = new char[newProductLen];
-        std::strncpy(newProduct, key.product(), newProductLen);
+        auto newProductTmp = new char[newProductLen];
+        std::strncpy(newProductTmp, key.product(), newProductLen);
+        newProduct = newProductTmp;
         labels_.push_back(newProduct);
       }
-      char* newProcess = kEmptyString;
+      char const* newProcess = kEmptyString;
       if (key.process()[0] != 0) {
         size_t newProcessLen = strlen(key.process()) + 1;
-        newProcess = new char[newProcessLen];
-        std::strncpy(newProcess, key.process(), newProcessLen);
+        auto newProcessTmp = new char[newProcessLen];
+        std::strncpy(newProcessTmp, key.process(), newProcessLen);
+        newProcess = newProcessTmp;
         labels_.push_back(newProcess);
       }
       internal::DataKey newKey(edm::TypeID(iInfo), newModule, newProduct, newProcess);
@@ -251,8 +253,9 @@ namespace fwlite {
 
       if (!foundProcessLabel.empty()) {
         //also remember it with the process label
-        newProcess = new char[foundProcessLabel.size() + 1];
-        std::strcpy(newProcess, foundProcessLabel.c_str());
+        auto newProcessTmp = new char[foundProcessLabel.size() + 1];
+        std::strcpy(newProcessTmp, foundProcessLabel.c_str());
+        newProcess = newProcessTmp;
         labels_.push_back(newProcess);
         internal::DataKey newKeyWithProcess(edm::TypeID(iInfo), newModule, newProduct, newProcess);
 

diff --git a/DataFormats/FWLite/src/IOVSyncValue.cc b/DataFormats/FWLite/src/IOVSyncValue.cc
@@ -70,17 +70,17 @@ namespace fwlite {
   // static member functions
   //
   const IOVSyncValue& IOVSyncValue::invalidIOVSyncValue() {
-    static IOVSyncValue s_invalid;
+    static const IOVSyncValue s_invalid;
     return s_invalid;
   }
   const IOVSyncValue& IOVSyncValue::endOfTime() {
-    static IOVSyncValue s_endOfTime(
+    static const IOVSyncValue s_endOfTime(
         edm::EventID(0xFFFFFFFFUL, edm::LuminosityBlockID::maxLuminosityBlockNumber(), edm::EventID::maxEventNumber()),
         edm::Timestamp::endOfTime());
     return s_endOfTime;
   }
   const IOVSyncValue& IOVSyncValue::beginOfTime() {
-    static IOVSyncValue s_beginOfTime(edm::EventID(1, 0, 0), edm::Timestamp::beginOfTime());
+    static const IOVSyncValue s_beginOfTime(edm::EventID(1, 0, 0), edm::Timestamp::beginOfTime());
     return s_beginOfTime;
   }
 }  // namespace fwlite