From f9cc0d2e5f46b0f887e3628c668ae00762e9ccd6 Mon Sep 17 00:00:00 2001 From: Andy Maloney Date: Thu, 11 Jan 2024 11:41:18 -0500 Subject: [PATCH] Split value range validation checks out of node implementation constructors (#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 --- src/E57XmlParser.cpp | 52 +++++++++++++++++++++++------------ src/FloatNode.cpp | 3 +- src/FloatNodeImpl.cpp | 17 ++++++------ src/FloatNodeImpl.h | 6 ++-- src/IntegerNode.cpp | 1 + src/IntegerNodeImpl.cpp | 14 ++++++---- src/IntegerNodeImpl.h | 2 ++ src/ScaledIntegerNode.cpp | 4 +++ src/ScaledIntegerNodeImpl.cpp | 27 ++++++------------ src/ScaledIntegerNodeImpl.h | 2 ++ 10 files changed, 76 insertions(+), 52 deletions(-) diff --git a/src/E57XmlParser.cpp b/src/E57XmlParser.cpp index 78444ed..9d90c78 100644 --- 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 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 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 f_ni( + new FloatNodeImpl( imf_, floatValue, pi.precision, pi.floatMinimum, pi.floatMaximum ) ); + + if ( foundValue ) + { + f_ni->validateValue(); } - std::shared_ptr 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 index b6ed72d..0de1811 100644 --- 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 index 0319b07..d22f17a 100644 --- 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 index 127b2b9..6d9a309 100644 --- a/src/FloatNodeImpl.h +++ b/src/FloatNodeImpl.h @@ -33,8 +33,8 @@ 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 @@ -42,6 +42,8 @@ namespace e57 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 index 293e46c..4401d84 100644 --- 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 index 373042a..6bb19cd 100644 --- 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 index 3c5a01c..425208c 100644 --- 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 index c20cf8d..7a174bd 100644 --- a/src/ScaledIntegerNode.cpp +++ b/src/ScaledIntegerNode.cpp @@ -153,6 +153,7 @@ 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, @@ -160,6 +161,7 @@ ScaledIntegerNode::ScaledIntegerNode( const ImageFile &destImageFile, int rawVal impl_( new ScaledIntegerNodeImpl( destImageFile.impl(), static_cast( 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( minimum ), static_cast( 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 index f90c69a..f35dd54 100644 --- 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( 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 index 54c93b2..fd8da63 100644 --- 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;