From 482cb2ded5002e673e58b64907a1b51ca0baf07e Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 26 Sep 2021 15:52:58 +0100 Subject: [PATCH 1/3] Clarify ownership model of CiffComponent::pData_ --- src/crwimage_int.cpp | 16 ++++------------ src/crwimage_int.hpp | 20 ++++++++++++++------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/crwimage_int.cpp b/src/crwimage_int.cpp index d007ad0e..71cb8347 100644 --- a/src/crwimage_int.cpp +++ b/src/crwimage_int.cpp @@ -176,9 +176,7 @@ namespace Exiv2 { } CiffComponent::~CiffComponent() - { - if (isAllocated_) delete[] pData_; - } + {} CiffDirectory::~CiffDirectory() { @@ -560,15 +558,9 @@ namespace Exiv2 { void CiffComponent::setValue(DataBuf buf) { - if (isAllocated_) { - delete[] pData_; - pData_ = nullptr; - size_ = 0; - } - isAllocated_ = true; - std::pair p = buf.release(); - pData_ = p.first; - size_ = p.second; + storage_ = buf; + pData_ = storage_.c_data(); + size_ = storage_.size(); if (size_ > 8 && dataLocation() == directoryData) { tag_ &= 0x3fff; } diff --git a/src/crwimage_int.hpp b/src/crwimage_int.hpp index 884a3ed0..97d2401a 100644 --- a/src/crwimage_int.hpp +++ b/src/crwimage_int.hpp @@ -87,12 +87,12 @@ namespace Exiv2 { //@{ //! Default constructor CiffComponent() - : dir_(0), tag_(0), size_(0), offset_(0), pData_(0), - isAllocated_(false) {} + : dir_(0), tag_(0), size_(0), offset_(0), pData_(0) + {} //! Constructor taking a tag and directory CiffComponent(uint16_t tag, uint16_t dir) - : dir_(dir), tag_(tag), size_(0), offset_(0), pData_(0), - isAllocated_(false) {} + : dir_(dir), tag_(tag), size_(0), offset_(0), pData_(0) + {} //! Virtual destructor. virtual ~CiffComponent(); //@} @@ -287,9 +287,17 @@ namespace Exiv2 { uint16_t tag_; //!< Tag of the entry uint32_t size_; //!< Size of the data area uint32_t offset_; //!< Offset to the data area from start of dir - const byte* pData_; //!< Pointer to the data area - bool isAllocated_; //!< True if this entry owns the value data + // Notes on the ownership model of pData_: pData_ is a always a read-only + // pointer to a buffer owned by somebody else. Usually it is a pointer + // into a copy of the image that was memory-mapped in CrwImage::readMetadata(). + // However, if CiffComponent::setValue() is used, then it is a pointer into + // the storage_ DataBuf below. + const byte* pData_; //!< Pointer to the data area + + // This DataBuf is only used when CiffComponent::setValue is called. + // Otherwise, it remains empty. + DataBuf storage_; }; // class CiffComponent /*! From 5fdd9da3d3bf5a094796a2b53a39d0e2cc8f9492 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sat, 9 Oct 2021 21:39:15 +0100 Subject: [PATCH 2/3] Use =default. --- src/crwimage_int.cpp | 3 --- src/crwimage_int.hpp | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/crwimage_int.cpp b/src/crwimage_int.cpp index 71cb8347..b0bc17b4 100644 --- a/src/crwimage_int.cpp +++ b/src/crwimage_int.cpp @@ -175,9 +175,6 @@ namespace Exiv2 { delete[] pPadding_; } - CiffComponent::~CiffComponent() - {} - CiffDirectory::~CiffDirectory() { for (auto&& component : components_) { diff --git a/src/crwimage_int.hpp b/src/crwimage_int.hpp index 97d2401a..c7f0041a 100644 --- a/src/crwimage_int.hpp +++ b/src/crwimage_int.hpp @@ -94,7 +94,7 @@ namespace Exiv2 { : dir_(dir), tag_(tag), size_(0), offset_(0), pData_(0) {} //! Virtual destructor. - virtual ~CiffComponent(); + virtual ~CiffComponent() = default; //@} //! @name Manipulators From 3b0d8270476a0e8050c88428eb6e493544abc5cb Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 10 Oct 2021 10:43:07 +0100 Subject: [PATCH 3/3] More =default changes. --- src/crwimage_int.hpp | 45 +++++++++++++++++--------------------------- 1 file changed, 17 insertions(+), 28 deletions(-) diff --git a/src/crwimage_int.hpp b/src/crwimage_int.hpp index c7f0041a..a5528933 100644 --- a/src/crwimage_int.hpp +++ b/src/crwimage_int.hpp @@ -86,12 +86,10 @@ namespace Exiv2 { //! @name Creators //@{ //! Default constructor - CiffComponent() - : dir_(0), tag_(0), size_(0), offset_(0), pData_(0) - {} + CiffComponent() = default; //! Constructor taking a tag and directory CiffComponent(uint16_t tag, uint16_t dir) - : dir_(dir), tag_(tag), size_(0), offset_(0), pData_(0) + : dir_(dir), tag_(tag) {} //! Virtual destructor. virtual ~CiffComponent() = default; @@ -283,17 +281,17 @@ namespace Exiv2 { private: // DATA - uint16_t dir_; //!< Tag of the directory containing this component - uint16_t tag_; //!< Tag of the entry - uint32_t size_; //!< Size of the data area - uint32_t offset_; //!< Offset to the data area from start of dir + uint16_t dir_ = 0; //!< Tag of the directory containing this component + uint16_t tag_ = 0; //!< Tag of the entry + uint32_t size_ = 0; //!< Size of the data area + uint32_t offset_ = 0; //!< Offset to the data area from start of dir // Notes on the ownership model of pData_: pData_ is a always a read-only // pointer to a buffer owned by somebody else. Usually it is a pointer // into a copy of the image that was memory-mapped in CrwImage::readMetadata(). // However, if CiffComponent::setValue() is used, then it is a pointer into // the storage_ DataBuf below. - const byte* pData_; //!< Pointer to the data area + const byte* pData_ = nullptr; //!< Pointer to the data area // This DataBuf is only used when CiffComponent::setValue is called. // Otherwise, it remains empty. @@ -309,12 +307,9 @@ namespace Exiv2 { //! @name Creators //@{ //! Default constructor - CiffEntry() {} + CiffEntry() = default; //! Constructor taking a tag and directory CiffEntry(uint16_t tag, uint16_t dir) : CiffComponent(tag, dir) {} - - //! Virtual destructor. - ~CiffEntry() override = default; //@} // Default assignment operator is fine @@ -346,9 +341,9 @@ namespace Exiv2 { //! @name Creators //@{ //! Default constructor - CiffDirectory() : cc_(NULL) {} + CiffDirectory() = default; //! Constructor taking a tag and directory - CiffDirectory(uint16_t tag, uint16_t dir) : CiffComponent(tag, dir), cc_(NULL) {} + CiffDirectory(uint16_t tag, uint16_t dir) : CiffComponent(tag, dir) {} //! Virtual destructor ~CiffDirectory() override; @@ -407,7 +402,7 @@ namespace Exiv2 { // DATA Components components_; //!< List of components in this dir UniquePtr m_; // used by recursive doAdd - CiffComponent* cc_; + CiffComponent* cc_ = nullptr; }; // class CiffDirectory @@ -425,13 +420,7 @@ namespace Exiv2 { //! @name Creators //@{ //! Default constructor - CiffHeader() - : pRootDir_ (0), - byteOrder_ (littleEndian), - offset_ (0x0000001a), - pPadding_ (0), - padded_ (0) - {} + CiffHeader() = default; //! Virtual destructor virtual ~CiffHeader(); //@} @@ -512,11 +501,11 @@ namespace Exiv2 { // DATA static const char signature_[]; //!< Canon CRW signature "HEAPCCDR" - CiffDirectory* pRootDir_; //!< Pointer to the root directory - ByteOrder byteOrder_; //!< Applicable byte order - uint32_t offset_; //!< Offset to the start of the root dir - byte* pPadding_; //!< Pointer to the (unknown) remainder - uint32_t padded_; //!< Number of padding-bytes + CiffDirectory* pRootDir_ = nullptr; //!< Pointer to the root directory + ByteOrder byteOrder_ = littleEndian; //!< Applicable byte order + uint32_t offset_ = 0; //!< Offset to the start of the root dir + byte* pPadding_ = nullptr; //!< Pointer to the (unknown) remainder + uint32_t padded_ = 0; //!< Number of padding-bytes }; // class CiffHeader