Split value range validation checks out of node implementation constr…

…uctors (#278) This means we don't have to add more arguments to constructors like we did with the float node implementation (which was also changed). When reading, we only call the validation if we actually read in a value for Integer, ScaledInteger, and Float nodes (see E57XmlParser.cpp). Fixes #275
asmaloney · Jan 11, 2024 · f9cc0d2 · f9cc0d2
1 parent 85518e0
commit f9cc0d2
Show file tree

Hide file tree

Showing 10 changed files with 76 additions and 52 deletions.
diff --git a/src/E57XmlParser.cpp b/src/E57XmlParser.cpp
@@ -713,52 +713,70 @@ void E57XmlParser::endElement( const XMLCh *const uri, const XMLCh *const localN
       break;
       case TypeInteger:
       {
-         // Convert child text (if any) to value, else default to 0.0
-         int64_t intValue;
+         // Convert child text (if any) to value, else default to 0
+         int64_t intValue = 0;
+         bool foundValue = false;
+
          if ( pi.childText.length() > 0 )
          {
             intValue = convertStrToLL( pi.childText );
+            foundValue = true;
          }
-         else
-         {
-            intValue = 0;
-         }
+
          std::shared_ptr<IntegerNodeImpl> i_ni(
             new IntegerNodeImpl( imf_, intValue, pi.minimum, pi.maximum ) );
+
+         if ( foundValue )
+         {
+            i_ni->validateValue();
+         }
+
          current_ni = i_ni;
       }
       break;
       case TypeScaledInteger:
       {
-         // Convert child text (if any) to value, else default to 0.0
-         int64_t intValue;
+         // Convert child text (if any) to value, else default to 0
+         int64_t intValue = 0;
+         bool foundValue = false;
+
          if ( pi.childText.length() > 0 )
          {
             intValue = convertStrToLL( pi.childText );
+            foundValue = true;
          }
-         else
-         {
-            intValue = 0;
-         }
+
          std::shared_ptr<ScaledIntegerNodeImpl> si_ni( new ScaledIntegerNodeImpl(
             imf_, intValue, pi.minimum, pi.maximum, pi.scale, pi.offset ) );
+
+         if ( foundValue )
+         {
+            si_ni->validateValue();
+         }
+
          current_ni = si_ni;
       }
       break;
       case TypeFloat:
       {
-         // Convert child text (if any) to value
+         // Convert child text (if any) to value, else default to 0.0
          double floatValue = 0.0;
-         bool validValue = false;
+         bool foundValue = false;
 
          if ( pi.childText.length() > 0 )
          {
             floatValue = strToDouble( pi.childText );
-            validValue = true;
+            foundValue = true;
+         }
+
+         std::shared_ptr<FloatNodeImpl> f_ni(
+            new FloatNodeImpl( imf_, floatValue, pi.precision, pi.floatMinimum, pi.floatMaximum ) );
+
+         if ( foundValue )
+         {
+            f_ni->validateValue();
          }
 
-         std::shared_ptr<FloatNodeImpl> f_ni( new FloatNodeImpl(
-            imf_, floatValue, validValue, pi.precision, pi.floatMinimum, pi.floatMaximum ) );
          current_ni = f_ni;
       }
       break;

diff --git a/src/FloatNode.cpp b/src/FloatNode.cpp
@@ -151,8 +151,9 @@ be true).
 */
 FloatNode::FloatNode( const ImageFile &destImageFile, double value, FloatPrecision precision,
                       double minimum, double maximum ) :
-   impl_( new FloatNodeImpl( destImageFile.impl(), value, true, precision, minimum, maximum ) )
+   impl_( new FloatNodeImpl( destImageFile.impl(), value, precision, minimum, maximum ) )
 {
+   impl_->validateValue();
 }
 
 /*!

diff --git a/src/FloatNodeImpl.cpp b/src/FloatNodeImpl.cpp
@@ -31,13 +31,11 @@
 
 namespace e57
 {
-   FloatNodeImpl::FloatNodeImpl( ImageFileImplWeakPtr destImageFile, double value, bool validValue,
+   FloatNodeImpl::FloatNodeImpl( ImageFileImplWeakPtr destImageFile, double value,
                                  FloatPrecision precision, double minimum, double maximum ) :
       NodeImpl( destImageFile ),
       value_( value ), precision_( precision ), minimum_( minimum ), maximum_( maximum )
    {
-      // don't checkImageFileOpen, NodeImpl() will do it
-
       // Since this ctor also used to construct single precision, and defaults for minimum/maximum
       // are for double precision, adjust bounds smaller if single.
       if ( precision_ == PrecisionSingle )
@@ -51,14 +49,17 @@ namespace e57
             maximum_ = FLOAT_MAX;
          }
       }
+   }
 
-      // Enforce the given bounds on raw value if it is valid
-      if ( validValue && ( value < minimum || value > maximum ) )
+   // Throw an exception if the value is not within bounds.
+   void FloatNodeImpl::validateValue() const
+   {
+      if ( value_ < minimum_ || value_ > maximum_ )
       {
          throw E57_EXCEPTION2( ErrorValueOutOfBounds, "this->pathName=" + this->pathName() +
-                                                         " value=" + toString( value ) +
-                                                         " minimum=" + toString( minimum ) +
-                                                         " maximum=" + toString( maximum ) );
+                                                         " value=" + toString( value_ ) +
+                                                         " minimum=" + toString( minimum_ ) +
+                                                         " maximum=" + toString( maximum_ ) );
       }
    }
 

diff --git a/src/FloatNodeImpl.h b/src/FloatNodeImpl.h
@@ -33,15 +33,17 @@ namespace e57
    class FloatNodeImpl : public NodeImpl
    {
    public:
-      FloatNodeImpl( ImageFileImplWeakPtr destImageFile, double value, bool validValue,
-                     FloatPrecision precision, double minimum, double maximum );
+      FloatNodeImpl( ImageFileImplWeakPtr destImageFile, double value, FloatPrecision precision,
+                     double minimum, double maximum );
       ~FloatNodeImpl() override = default;
 
       NodeType type() const override
       {
          return TypeFloat;
       }
 
+      void validateValue() const;
+
       bool isTypeEquivalent( NodeImplSharedPtr ni ) override;
       bool isDefined( const ustring &pathName ) override;
 

diff --git a/src/IntegerNode.cpp b/src/IntegerNode.cpp
@@ -146,6 +146,7 @@ IntegerNode::IntegerNode( const ImageFile &destImageFile, int64_t value, int64_t
                           int64_t maximum ) :
    impl_( new IntegerNodeImpl( destImageFile.impl(), value, minimum, maximum ) )
 {
+   impl_->validateValue();
 }
 
 /*!

diff --git a/src/IntegerNodeImpl.cpp b/src/IntegerNodeImpl.cpp
@@ -36,15 +36,17 @@ namespace e57
       NodeImpl( destImageFile ),
       value_( value ), minimum_( minimum ), maximum_( maximum )
    {
-      // don't checkImageFileOpen, NodeImpl() will do it
+   }
 
-      // Enforce the given bounds
-      if ( value < minimum || maximum < value )
+   // Throw an exception if the value is not within bounds.
+   void IntegerNodeImpl::validateValue() const
+   {
+      if ( value_ < minimum_ || value_ > maximum_ )
       {
          throw E57_EXCEPTION2( ErrorValueOutOfBounds, "this->pathName=" + this->pathName() +
-                                                         " value=" + toString( value ) +
-                                                         " minimum=" + toString( minimum ) +
-                                                         " maximum=" + toString( maximum ) );
+                                                         " value=" + toString( value_ ) +
+                                                         " minimum=" + toString( minimum_ ) +
+                                                         " maximum=" + toString( maximum_ ) );
       }
    }
 

diff --git a/src/IntegerNodeImpl.h b/src/IntegerNodeImpl.h
@@ -42,6 +42,8 @@ namespace e57
          return TypeInteger;
       }
 
+      void validateValue() const;
+
       bool isTypeEquivalent( NodeImplSharedPtr ni ) override;
       bool isDefined( const ustring &pathName ) override;
 

diff --git a/src/ScaledIntegerNode.cpp b/src/ScaledIntegerNode.cpp
@@ -153,13 +153,15 @@ ScaledIntegerNode::ScaledIntegerNode( const ImageFile &destImageFile, int64_t ra
    impl_(
       new ScaledIntegerNodeImpl( destImageFile.impl(), rawValue, minimum, maximum, scale, offset ) )
 {
+   impl_->validateValue();
 }
 
 ScaledIntegerNode::ScaledIntegerNode( const ImageFile &destImageFile, int rawValue, int64_t minimum,
                                       int64_t maximum, double scale, double offset ) :
    impl_( new ScaledIntegerNodeImpl( destImageFile.impl(), static_cast<int64_t>( rawValue ),
                                      minimum, maximum, scale, offset ) )
 {
+   impl_->validateValue();
 }
 
 ScaledIntegerNode::ScaledIntegerNode( const ImageFile &destImageFile, int rawValue, int minimum,
@@ -168,6 +170,7 @@ ScaledIntegerNode::ScaledIntegerNode( const ImageFile &destImageFile, int rawVal
                                      static_cast<int64_t>( minimum ),
                                      static_cast<int64_t>( maximum ), scale, offset ) )
 {
+   impl_->validateValue();
 }
 
 /*!
@@ -212,6 +215,7 @@ ScaledIntegerNode::ScaledIntegerNode( const ImageFile &destImageFile, double sca
    impl_( new ScaledIntegerNodeImpl( destImageFile.impl(), scaledValue, scaledMinimum,
                                      scaledMaximum, scale, offset ) )
 {
+   impl_->validateValue();
 }
 
 /*!

diff --git a/src/ScaledIntegerNodeImpl.cpp b/src/ScaledIntegerNodeImpl.cpp
@@ -40,16 +40,6 @@ namespace e57
       value_( rawValue ), minimum_( minimum ), maximum_( maximum ), scale_( scale ),
       offset_( offset )
    {
-      // don't checkImageFileOpen, NodeImpl() will do it
-
-      // Enforce the given bounds on raw value
-      if ( rawValue < minimum || maximum < rawValue )
-      {
-         throw E57_EXCEPTION2( ErrorValueOutOfBounds, "this->pathName=" + this->pathName() +
-                                                         " rawValue=" + toString( rawValue ) +
-                                                         " minimum=" + toString( minimum ) +
-                                                         " maximum=" + toString( maximum ) );
-      }
    }
 
    ScaledIntegerNodeImpl::ScaledIntegerNodeImpl( ImageFileImplWeakPtr destImageFile,
@@ -62,16 +52,17 @@ namespace e57
       maximum_( static_cast<int64_t>( std::floor( ( scaledMaximum - offset ) / scale + .5 ) ) ),
       scale_( scale ), offset_( offset )
    {
-      // don't checkImageFileOpen, NodeImpl() will do it
+   }
 
-      // Enforce the given bounds on raw value
-      if ( scaledValue < scaledMinimum || scaledMaximum < scaledValue )
+   // Throw an exception if the value is not within bounds.
+   void ScaledIntegerNodeImpl::validateValue() const
+   {
+      if ( value_ < minimum_ || value_ > maximum_ )
       {
-         throw E57_EXCEPTION2( ErrorValueOutOfBounds,
-                               "this->pathName=" + this->pathName() +
-                                  " scaledValue=" + toString( scaledValue ) +
-                                  " scaledMinimum=" + toString( scaledMinimum ) +
-                                  " scaledMaximum=" + toString( scaledMaximum ) );
+         throw E57_EXCEPTION2( ErrorValueOutOfBounds, "this->pathName=" + this->pathName() +
+                                                         " value=" + toString( value_ ) +
+                                                         " minimum=" + toString( minimum_ ) +
+                                                         " maximum=" + toString( maximum_ ) );
       }
    }
 

diff --git a/src/ScaledIntegerNodeImpl.h b/src/ScaledIntegerNodeImpl.h
@@ -47,6 +47,8 @@ namespace e57
          return TypeScaledInteger;
       }
 
+      void validateValue() const;
+
       bool isTypeEquivalent( NodeImplSharedPtr ni ) override;
       bool isDefined( const ustring &pathName ) override;