From f90f8b758c50d754beec8f729c5e87e2b029c724 Mon Sep 17 00:00:00 2001
From: Markus Frank <Markus.Frank@cern.ch>
Date: Thu, 10 Jan 2019 17:23:35 +0100
Subject: [PATCH] Improve and fix conditions handling

---
 DDCond/src/plugins/ConditionsUserPool.cpp     |  2 +-
 DDCore/include/DD4hep/BasicGrammar.h          | 52 +++++++++++++
 DDCore/include/DD4hep/Conditions.h            | 52 ++++++-------
 DDCore/include/DD4hep/OpaqueData.h            | 37 ++++++---
 .../include/DD4hep/detail/BasicGrammar_inl.h  |  5 ++
 .../include/DD4hep/detail/ConditionsInterna.h |  2 +-
 DDCore/include/DD4hep/detail/Grammar.h        |  6 +-
 DDCore/include/DD4hep/detail/OpaqueData_inl.h |  4 +-
 DDCore/src/BasicGrammar.cpp                   | 42 ++++++++++
 DDCore/src/OpaqueData.cpp                     | 76 ++++++++++++-------
 .../compact/Check_Shape_PseudoTrap3.xml       |  2 +-
 11 files changed, 213 insertions(+), 67 deletions(-)

diff --git a/DDCond/src/plugins/ConditionsUserPool.cpp b/DDCond/src/plugins/ConditionsUserPool.cpp
index 45cf45ffd..842289d2d 100644
--- a/DDCond/src/plugins/ConditionsUserPool.cpp
+++ b/DDCond/src/plugins/ConditionsUserPool.cpp
@@ -387,8 +387,8 @@ ConditionsMappedUserPool<MAPPING>::get(Condition::key_type lower, Condition::key
   if ( !m_conditions.empty() )   {
     typename MAPPING::const_iterator first = m_conditions.lower_bound(lower);
     for(; first != m_conditions.end(); ++first )  {
-      result.push_back((*first).second);
       if ( (*first).first > upper ) break;
+      result.push_back((*first).second);
     }
   }
   return result;
diff --git a/DDCore/include/DD4hep/BasicGrammar.h b/DDCore/include/DD4hep/BasicGrammar.h
index b28b1c6fd..4e817be24 100644
--- a/DDCore/include/DD4hep/BasicGrammar.h
+++ b/DDCore/include/DD4hep/BasicGrammar.h
@@ -90,6 +90,8 @@ namespace dd4hep {
       return initialized_clazz();
     }
     /// Access to the type information
+    virtual bool equals(const std::type_info& other_type) const = 0;
+    /// Access to the type information
     virtual const std::type_info& type() const = 0;
     /// Access the object size (sizeof operator)
     virtual size_t sizeOf() const = 0;
@@ -104,6 +106,56 @@ namespace dd4hep {
     /// Bind opaque address to object
     virtual void bind(void* pointer)  const = 0;
   };
+
+  /// Concrete type dependent simple grammar definition. Common functionality
+  /**
+   *   Note: All methods below only throw exceptions. This "simple" grammar 
+   *   cannot answer them. There is also no string parsing implemented for these.
+   *
+   *   \author  M.Frank
+   *   \date    13.08.2013
+   *   \ingroup DD4HEP
+   */
+  class SimpleGrammar : public BasicGrammar {
+  protected:
+    
+    friend class BasicGrammar;
+
+    size_t                object_size;
+    /// Reference to type information
+    const std::type_info& object_type;
+    
+    /// Standard constructor
+    SimpleGrammar(size_t siz, const std::type_info& typ);
+    /// Default destructor
+    virtual ~SimpleGrammar();
+
+  public:
+    template <typename TYPE> static BasicGrammar* instance()  {
+      static SimpleGrammar s_grammar(sizeof(TYPE), typeid(TYPE));
+      return &s_grammar;
+    }
+    /** Base class overrides   */
+    /// PropertyGrammar overload: Access to the type information
+    virtual const std::type_info& type() const  override   {  return object_type;   }
+    /// Access the object size (sizeof operator)
+    virtual size_t sizeOf() const  override                {  return object_size;   }
+    /// Access to the type information
+    virtual bool equals(const std::type_info& other_type) const  override
+    {  return other_type == object_type;  }
+    /** Base class overrides   */
+    /// PropertyGrammar overload: Serialize a property to a string
+    virtual std::string str(const void* ptr) const  override;
+    /// PropertyGrammar overload: Retrieve value from string
+    virtual bool fromString(void* ptr, const std::string& value) const  override;
+    /// Opaque object destructor
+    virtual void destruct(void* pointer) const  override;
+    /// Opaque object copy construction. Memory must be allocated externally
+    virtual void copy(void* to, const void* from)  const  override;
+    /// Bind opaque address to object
+    virtual void bind(void* pointer)  const override;
+  };  
+
 }      // End namespace dd4hep
 
 #endif  /* DD4HEP_DDG4_BASICGRAMMAR_H */
diff --git a/DDCore/include/DD4hep/Conditions.h b/DDCore/include/DD4hep/Conditions.h
index 4156c1607..7600b8761 100644
--- a/DDCore/include/DD4hep/Conditions.h
+++ b/DDCore/include/DD4hep/Conditions.h
@@ -62,48 +62,48 @@ namespace dd4hep {
   public:
     /// Flags to steer the conditions conversion to string
     enum StringFlags  {
-      WITH_IOV           = 1<<0,
-      WITH_ADDRESS       = 1<<1,
-      WITH_TYPE          = 1<<2,
-      WITH_COMMENT       = 1<<4,
-      WITH_DATATYPE      = 1<<5,
-      WITH_DATA          = 1<<6,
-      NO_NAME            = 1<<20,
+      WITH_IOV            =  1<<0,
+      WITH_ADDRESS        =  1<<1,
+      WITH_TYPE           =  1<<2,
+      WITH_COMMENT        =  1<<4,
+      WITH_DATATYPE       =  1<<5,
+      WITH_DATA           =  1<<6,
+      NO_NAME             =  1<<20,
       NONE
     };
     /// Flags to indicate the conditions type ans state
     enum ConditionState {
-      INACTIVE            = 0,
-      ACTIVE              = 1<<0,
-      CHECKED             = 1<<2,
-      DERIVED             = 1<<3,
-      ONSTACK             = 1<<4,
+      INACTIVE            =  0,
+      ACTIVE              =  1<<0,
+      CHECKED             =  1<<2,
+      DERIVED             =  1<<3,
+      ONSTACK             =  1<<4,
       // Flags for specific conditions
-      TEMPERATURE         = 1<<5,
-      TEMPERATURE_DERIVED = 1<<6|DERIVED,
-      PRESSURE            = 1<<7,
-      PRESSURE_DERIVED    = 1<<8|DERIVED,
-      ALIGNMENT_DELTA     = 1<<9,
-      ALIGNMENT_DERIVED   = 1<<10|DERIVED,
+      TEMPERATURE         =  1<<5,
+      TEMPERATURE_DERIVED =  1<<6|DERIVED,
+      PRESSURE            =  1<<7,
+      PRESSURE_DERIVED    =  1<<8|DERIVED,
+      ALIGNMENT_DELTA     =  1<<9,
+      ALIGNMENT_DERIVED   =  1<<10|DERIVED,
       // Keep bit 10-15 for other generic types
       // Bit 16-31 is reserved for user classifications
-      USER_FLAGS_FIRST    = 1<<16,
-      USER_FLAGS_LAST     = 1<<31
+      USER_FLAGS_FIRST    =  1<<16,
+      USER_FLAGS_LAST     =  1<<31
     };
     /// Flags to indicate conditions item ranges (low word of the conditions key)
     enum ConditionItemRangeKeys {
-      FIRST_ITEM_KEY =  0x0U,
-      LAST_ITEM_KEY  = ~0x0U
+      FIRST_ITEM_KEY      =  0x0U,
+      LAST_ITEM_KEY       = ~0x0U
     };
     /// Flags to indicate conditions detector ranges (high word of the conditions key)
     enum ConditionDetectorRangeKeys {
-      FIRST_DET_KEY =  0x0U,
-      LAST_DET_KEY  = ~0x0U
+      FIRST_DET_KEY       =  0x0U,
+      LAST_DET_KEY        = ~0x0U
     };
     /// Flags to indicate global conditions ranges
     enum {
-      FIRST_KEY  =  0x0ULL,
-      LAST_KEY   = ~0x0ULL        
+      FIRST_KEY           =  0x0ULL,
+      LAST_KEY            = ~0x0ULL        
     };
 
     /// Abstract base for processing callbacks to conditions objects
diff --git a/DDCore/include/DD4hep/OpaqueData.h b/DDCore/include/DD4hep/OpaqueData.h
index 6e5b444d5..ac51863ed 100644
--- a/DDCore/include/DD4hep/OpaqueData.h
+++ b/DDCore/include/DD4hep/OpaqueData.h
@@ -85,20 +85,26 @@ namespace dd4hep {
   class OpaqueDataBlock : public OpaqueData   {
 
   protected:
-    enum _DataTypes  {
-      PLAIN_DATA = 1<<0,
-      ALLOC_DATA = 1<<1,
-      STACK_DATA = 1<<2,
-      BOUND_DATA = 1<<3
-    };
-
     /// Data buffer: plain data are allocated directly on this buffer
-    /** Internal data buffer is sufficient to store any vector  */
+    /** This internal data buffer is sufficient to store any 
+     *  STL vector, list, map, etc. and hence should be sufficient to
+     *  probably store normal relatively primitive basic objects.
+     */
     unsigned char data[sizeof(std::vector<void*>)];
 
   public:
-    /// Data buffer type: Must be a bitmap!
+    enum _DataTypes  {
+      PLAIN_DATA  =  1<<0,
+      ALLOC_DATA  =  1<<1,
+      STACK_DATA  =  1<<2,
+      BOUND_DATA  =  1<<3,
+      EXTERN_DATA =  1<<4
+    };
+
+    /// Data buffer type: Must be a bitmap of enum _DataTypes!
     unsigned int type;
+
+  public:
     /// Standard initializing constructor
     OpaqueDataBlock();
     /// Copy constructor
@@ -115,6 +121,10 @@ namespace dd4hep {
     void* bind(const BasicGrammar* grammar);
     /// Bind data value in place
     void* bind(void* ptr, size_t len, const BasicGrammar* grammar);
+    /// Bind external data value to the pointer
+    void bindExtern(void* ptr, const BasicGrammar* grammar);
+    /// Bind external data value to the pointer
+    template <typename T> void bindExtern(T* ptr);
     /// Bind data value
     template <typename T> T& bind();
     /// Bind data value
@@ -126,6 +136,15 @@ namespace dd4hep {
     /// Set data value
     void assign(const void* ptr,const std::type_info& typ);
   };
+}      /* End namespace dd4hep */
+
+#include "DD4hep/BasicGrammar.h"
+
+/// Namespace for the AIDA detector description toolkit
+namespace dd4hep {
 
+  template <typename T> inline void OpaqueDataBlock::bindExtern(T* ptr)   {
+    this->OpaqueDataBlock::bindExtern((void*)ptr, SimpleGrammar::instance<T>());
+  }
 }      /* End namespace dd4hep */
 #endif /* DD4HEP_OPAQUEDATA_H  */
diff --git a/DDCore/include/DD4hep/detail/BasicGrammar_inl.h b/DDCore/include/DD4hep/detail/BasicGrammar_inl.h
index 559840732..485fa5a8d 100644
--- a/DDCore/include/DD4hep/detail/BasicGrammar_inl.h
+++ b/DDCore/include/DD4hep/detail/BasicGrammar_inl.h
@@ -65,6 +65,11 @@ namespace dd4hep {
     return typeid(TYPE);
   }
 
+  /// Access to the type information
+  template <typename TYPE> bool Grammar<TYPE>::equals(const std::type_info& other_type) const  {
+    return other_type == typeid(TYPE);
+  }
+  
   /// Access the object size (sizeof operator)
   template <typename TYPE> size_t Grammar<TYPE>::sizeOf() const   {
     return sizeof(TYPE);
diff --git a/DDCore/include/DD4hep/detail/ConditionsInterna.h b/DDCore/include/DD4hep/detail/ConditionsInterna.h
index 3a6e582db..ef3f8d728 100644
--- a/DDCore/include/DD4hep/detail/ConditionsInterna.h
+++ b/DDCore/include/DD4hep/detail/ConditionsInterna.h
@@ -155,7 +155,7 @@ namespace dd4hep {
 
 #define DD4HEP_DEFINE_EXTERNAL_CONDITIONS_TYPE(x)                 \
   namespace dd4hep {                                              \
-      template <> x& Condition::bind<x>(const std::string& val);  \
+    template <> x& Condition::bind<x>(const std::string& val);    \
       template <> x& Condition::bind<x>();                        \
       template <> x& Condition::get<x>();                         \
       template <> const x& Condition::get<x>() const;             \
diff --git a/DDCore/include/DD4hep/detail/Grammar.h b/DDCore/include/DD4hep/detail/Grammar.h
index 51803796b..a2b6f79ef 100644
--- a/DDCore/include/DD4hep/detail/Grammar.h
+++ b/DDCore/include/DD4hep/detail/Grammar.h
@@ -34,10 +34,12 @@ namespace dd4hep {
    *   \ingroup DD4HEP
    */
   template <typename TYPE> class Grammar : public BasicGrammar {
+  private:
+    
     friend class BasicGrammar;
     /// Default destructor
     virtual ~Grammar();
-    /// Standarsd constructor
+    /// Standard constructor
     Grammar();
 
   public:
@@ -45,6 +47,8 @@ namespace dd4hep {
     /** Base class overrides   */
     /// PropertyGrammar overload: Access to the type information
     virtual const std::type_info& type() const  override;
+    /// Access to the type information
+    virtual bool equals(const std::type_info& other_type) const  override;
     /// Access the object size (sizeof operator)
     virtual size_t sizeOf() const  override;
     /// PropertyGrammar overload: Serialize a property to a string
diff --git a/DDCore/include/DD4hep/detail/OpaqueData_inl.h b/DDCore/include/DD4hep/detail/OpaqueData_inl.h
index c63b4f3e7..21397a6f6 100644
--- a/DDCore/include/DD4hep/detail/OpaqueData_inl.h
+++ b/DDCore/include/DD4hep/detail/OpaqueData_inl.h
@@ -31,13 +31,13 @@ namespace dd4hep {
 
   /// Generic getter. Specify the exact type, not a polymorph type
   template <typename T> T& OpaqueData::get() {
-    if (!grammar || (grammar->type() != typeid(T))) { throw std::bad_cast(); }
+    if (!grammar || !grammar->equals(typeid(T))) { throw std::bad_cast(); }
     return *(T*)pointer;
   }
 
   /// Generic getter (const version). Specify the exact type, not a polymorph type
   template <typename T> const T& OpaqueData::get() const {
-    if (!grammar || (grammar->type() != typeid(T))) { throw std::bad_cast(); }
+    if (!grammar || !grammar->equals(typeid(T))) { throw std::bad_cast(); }
     return *(T*)pointer;
   }
 
diff --git a/DDCore/src/BasicGrammar.cpp b/DDCore/src/BasicGrammar.cpp
index d55ffac8b..bca5abee2 100644
--- a/DDCore/src/BasicGrammar.cpp
+++ b/DDCore/src/BasicGrammar.cpp
@@ -16,6 +16,7 @@
 #include "DD4hep/Primitives.h"
 #include "DD4hep/Exceptions.h"
 #include "DD4hep/BasicGrammar.h"
+#include "DD4hep/detail/Grammar.h"
 
 // ROOT include files
 #include "TDataType.h"
@@ -108,3 +109,44 @@ void dd4hep::BasicGrammar::invalidConversion(const std::type_info& from, const s
                              "' to '" + to_name + "' is not implemented.");
 }
 
+/// Standard constructor
+dd4hep::SimpleGrammar::SimpleGrammar(size_t siz, const std::type_info& typ)
+  : BasicGrammar(dd4hep::typeName(typ)), object_size(siz), object_type(typ)
+{
+}
+
+/// Default destructor
+dd4hep::SimpleGrammar::~SimpleGrammar()   {
+}
+
+/// PropertyGrammar overload: Serialize a property to a string
+std::string dd4hep::SimpleGrammar::str(const void* /* ptr */) const    {
+  dd4hep::except("SimpleGrammar","+++ The string representation str() is not implemented for grammar type %s",
+                 this->type_name().c_str());
+  return "";
+}
+
+/// PropertyGrammar overload: Retrieve value from string
+bool dd4hep::SimpleGrammar::fromString(void* /* ptr */, const std::string& /* value */) const    {
+  dd4hep::except("SimpleGrammar","+++ The data parsing from string is not implemented for grammar type %s",
+                 this->type_name().c_str());
+  return false;
+}
+
+/// Opaque object destructor
+void dd4hep::SimpleGrammar::destruct(void* /* pointer */) const    {
+  dd4hep::except("SimpleGrammar","+++ Object destruction is not implemented for grammar type %s",
+                 this->type_name().c_str());
+}
+
+/// Opaque object copy construction. Memory must be allocated externally
+void dd4hep::SimpleGrammar::copy(void* /* to */, const void* /* from */)  const    {
+  dd4hep::except("SimpleGrammar","+++ Object copy is not implemented for grammar type %s",
+                 this->type_name().c_str());
+}
+
+/// Bind opaque address to object
+void dd4hep::SimpleGrammar::bind(void* /* pointer */)  const   {
+  dd4hep::except("SimpleGrammar","+++ Object destruction is not implemented for grammar type %s",
+                 this->type_name().c_str());
+}
diff --git a/DDCore/src/OpaqueData.cpp b/DDCore/src/OpaqueData.cpp
index 16c2d0701..992b9f8c3 100644
--- a/DDCore/src/OpaqueData.cpp
+++ b/DDCore/src/OpaqueData.cpp
@@ -57,7 +57,7 @@ const string& OpaqueData::dataType() const   {
 }
 
 /// Standard initializing constructor
-OpaqueDataBlock::OpaqueDataBlock() : OpaqueData(), /*destruct(0), copy(0),*/ type(0)   {
+OpaqueDataBlock::OpaqueDataBlock() : OpaqueData(), type(0)   {
   InstanceCount::increment(this);
 }
 
@@ -73,7 +73,7 @@ OpaqueDataBlock::OpaqueDataBlock(const OpaqueDataBlock& c)
 
 /// Standard Destructor
 OpaqueDataBlock::~OpaqueDataBlock()   {
-  if ( pointer )  {
+  if ( pointer && (type&EXTERN_DATA) != EXTERN_DATA )  {
     grammar->destruct(pointer);
     if ( (type&ALLOC_DATA) == ALLOC_DATA ) ::operator delete(pointer);
   }
@@ -82,46 +82,55 @@ OpaqueDataBlock::~OpaqueDataBlock()   {
   InstanceCount::decrement(this);
 }
 
-/// Move the data content: 'from' will be reset to NULL
-bool OpaqueDataBlock::move(OpaqueDataBlock& from)   {
-  pointer = from.pointer;
-  grammar = from.grammar;
-  ::memcpy(data,from.data,sizeof(data));
-  type = from.type;
-  ::memset(from.data,0,sizeof(data));
-  from.type = PLAIN_DATA;
-  from.pointer = 0;
-  from.grammar = 0;
-  return true;
-}
-
 /// Copy constructor
 OpaqueDataBlock& OpaqueDataBlock::operator=(const OpaqueDataBlock& c)   {
   if ( this != &c )  {
-    if ( this->grammar == c.grammar )   {
+    if ( grammar == c.grammar )   {
       if ( pointer )  {
-        grammar->destruct(pointer);
+        if ( grammar ) grammar->destruct(pointer);
         if ( (type&ALLOC_DATA) == ALLOC_DATA ) ::operator delete(pointer);
       }
       pointer = 0;
       grammar = 0;
     }
-    if ( this->grammar == 0 )  {
+    if ( grammar == 0 )  {
       this->OpaqueData::operator=(c);
-      this->type = c.type;
-      this->grammar = 0;
-      this->bind(c.grammar);
-      this->grammar->copy(pointer,c.pointer);
-      return *this;
+      type = c.type;
+      grammar = 0;
+      if ( c.grammar )   {
+        bind(c.grammar);
+        grammar->copy(pointer,c.pointer);
+        return *this;
+      }
+      else if ( (c.type&EXTERN_DATA) == EXTERN_DATA )   {
+        pointer = c.pointer;
+        return *this;
+      }
     }
     except("OpaqueData","You may not bind opaque data multiple times!");
   }
   return *this;
 }
 
+/// Move the data content: 'from' will be reset to NULL
+bool OpaqueDataBlock::move(OpaqueDataBlock& from)   {
+  pointer = from.pointer;
+  grammar = from.grammar;
+  ::memcpy(data,from.data,sizeof(data));
+  type = from.type;
+  ::memset(from.data,0,sizeof(data));
+  from.type = PLAIN_DATA;
+  from.pointer = 0;
+  from.grammar = 0;
+  return true;
+}
+
 /// Bind data value
 void* OpaqueDataBlock::bind(const BasicGrammar* g)   {
-  if ( !grammar )  {
+  if ( (type&EXTERN_DATA) == EXTERN_DATA )  {
+    except("OpaqueData","Extern data may not be bound!");
+  }
+  else if ( !grammar )  {
     size_t len = g->sizeOf();
     grammar  = g;
     (len > sizeof(data))
@@ -140,7 +149,10 @@ void* OpaqueDataBlock::bind(const BasicGrammar* g)   {
 
 /// Set data value
 void* OpaqueDataBlock::bind(void* ptr, size_t size, const BasicGrammar* g)   {
-  if ( !grammar )  {
+  if ( (type&EXTERN_DATA) == EXTERN_DATA )  {
+    except("OpaqueData","Extern data may not be bound!");
+  }
+  else if ( !grammar )  {
     size_t len = g->sizeOf();
     grammar = g;
     if ( len <= size )
@@ -154,12 +166,24 @@ void* OpaqueDataBlock::bind(void* ptr, size_t size, const BasicGrammar* g)   {
   else if ( grammar == g )  {
     // We cannot ingore secondary requests for data bindings.
     // This leads to memory leaks in the caller!
-    except("OpaqueData","You may not bind opaque multiple times!");
+    except("OpaqueData","You may not bind opaque data multiple times!");
   }
   typeinfoCheck(grammar->type(),g->type(),"Opaque data blocks may not be assigned.");
   return 0;
 }
 
+/// Bind external data value to the pointer
+void OpaqueDataBlock::bindExtern(void* ptr, const BasicGrammar* gr)    {
+  if ( grammar != 0 && type != EXTERN_DATA )  {
+    // We cannot ingore secondary requests for data bindings.
+    // This leads to memory leaks in the caller!
+    except("OpaqueData","You may not bind opaque data multiple times!");
+  }
+  pointer = ptr;
+  grammar = gr;
+  type    = EXTERN_DATA;
+}
+
 /// Set data value
 void OpaqueDataBlock::assign(const void* ptr, const type_info& typ)  {
   if ( !grammar )   {
diff --git a/examples/ClientTests/compact/Check_Shape_PseudoTrap3.xml b/examples/ClientTests/compact/Check_Shape_PseudoTrap3.xml
index 6a48c8098..67646b615 100644
--- a/examples/ClientTests/compact/Check_Shape_PseudoTrap3.xml
+++ b/examples/ClientTests/compact/Check_Shape_PseudoTrap3.xml
@@ -47,7 +47,7 @@
       <!--   <PseudoTrap name="ptrap4" dx1="10*cm" dx2="3*cm" dy1="30*cm" dy2="10*cm" dz="30*cm" radius="-10*cm" atMinusZ="true"/>     -->
       <check vis="Shape4_vis">
         <shape type="PseudoTrap" name="ptrap4" 
-               x1="10*cm" x2="3*cm" 
+               x1="10*cm" x2="10*cm" 
                y1="30*cm" y2="10*cm" 
                z="30*cm"  radius="-10*cm" minusZ="true"/>
         <position x="50*cm"  y="-10*cm" z="0*cm"/>
-- 
GitLab