Skip to content

Commit

Permalink
Merge pull request #3179 from kevinbackhouse/tiffcomposite-unique_ptr
Browse files Browse the repository at this point in the history 
Use std::unique_ptr rather than raw pointer
  • Loading branch information
@kevinbackhouse
kevinbackhouse authored Feb 20, 2025
2 parents 9f8e1a5 + 52925b8 commit 0098d71
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 75 deletions.
67 changes: 23 additions & 44 deletions src/tiffcomposite_int.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ TiffIfdMakernote::TiffIfdMakernote(uint16_t tag, IfdId group, IfdId mnGroup, std
TiffComponent(tag, group), pHeader_(std::move(pHeader)), ifd_(tag, mnGroup, hasNext) {
}

TiffIfdMakernote::~TiffIfdMakernote() = default;

TiffBinaryArray::TiffBinaryArray(uint16_t tag, IfdId group, const ArrayCfg& arrayCfg, const ArrayDef* arrayDef,
size_t defSize) :
TiffEntryBase(tag, group, arrayCfg.elTiffType_), arrayCfg_(&arrayCfg), arrayDef_(arrayDef), defSize_(defSize) {
Expand All @@ -89,29 +91,6 @@ TiffBinaryArray::TiffBinaryArray(uint16_t tag, IfdId group, const ArraySet* arra
// We'll figure out the correct cfg later
}

TiffDirectory::~TiffDirectory() {
for (auto&& component : components_) {
delete component;
}
delete pNext_;
}

TiffSubIfd::~TiffSubIfd() {
for (auto&& ifd : ifds_) {
delete ifd;
}
}

TiffEntryBase::~TiffEntryBase() {
delete pValue_;
}

TiffBinaryArray::~TiffBinaryArray() {
for (auto&& element : elements_) {
delete element;
}
}

TiffEntryBase::TiffEntryBase(const TiffEntryBase& rhs) :
TiffComponent(rhs),
tiffType_(rhs.tiffType_),
Expand Down Expand Up @@ -228,8 +207,7 @@ void TiffEntryBase::setValue(Value::UniquePtr value) {
return;
tiffType_ = toTiffType(value->typeId());
count_ = value->count();
delete pValue_;
pValue_ = value.release();
pValue_ = std::move(value);
}

void TiffDataEntry::setStrips(const Value* pSize, const byte* pData, size_t sizeData, size_t baseOffset) {
Expand Down Expand Up @@ -446,11 +424,11 @@ TiffComponent* TiffDirectory::doAddPath(uint16_t tag, TiffPath& tiffPath, TiffCo
// condition takes care of them, see below.
if (tiffPath.size() > 1 || (tpi.extendedTag() == 0x927c && tpi.group() == IfdId::exifId)) {
if (tpi.extendedTag() == Tag::next) {
tc = pNext_;
tc = pNext_.get();
} else {
for (auto&& component : components_) {
if (component->tag() == tpi.tag() && component->group() == tpi.group()) {
tc = component;
tc = component.get();
break;
}
}
Expand Down Expand Up @@ -576,14 +554,17 @@ TiffComponent* TiffComponent::doAddChild(UniquePtr /*tiffComponent*/) {
} // TiffComponent::doAddChild

TiffComponent* TiffDirectory::doAddChild(TiffComponent::UniquePtr tiffComponent) {
TiffComponent* tc = tiffComponent.release();
components_.push_back(tc);
return tc;
components_.push_back(std::move(tiffComponent));
return components_.back().get();
} // TiffDirectory::doAddChild

TiffComponent* TiffSubIfd::doAddChild(TiffComponent::UniquePtr tiffComponent) {
auto d = dynamic_cast<TiffDirectory*>(tiffComponent.release());
ifds_.push_back(d);
auto d = dynamic_cast<TiffDirectory*>(tiffComponent.get());
if (!d) {
throw Error(ErrorCode::kerErrorMessage, "dynamic_cast to TiffDirectory failed");
}
tiffComponent.release();
ifds_.emplace_back(d);
return d;
} // TiffSubIfd::doAddChild

Expand All @@ -600,10 +581,9 @@ TiffComponent* TiffIfdMakernote::doAddChild(TiffComponent::UniquePtr tiffCompone
}

TiffComponent* TiffBinaryArray::doAddChild(TiffComponent::UniquePtr tiffComponent) {
TiffComponent* tc = tiffComponent.release();
elements_.push_back(tc);
elements_.push_back(std::move(tiffComponent));
setDecoded(true);
return tc;
return elements_.back().get();
} // TiffBinaryArray::doAddChild

TiffComponent* TiffComponent::addNext(TiffComponent::UniquePtr tiffComponent) {
Expand All @@ -615,12 +595,11 @@ TiffComponent* TiffComponent::doAddNext(UniquePtr /*tiffComponent*/) {
} // TiffComponent::doAddNext

TiffComponent* TiffDirectory::doAddNext(TiffComponent::UniquePtr tiffComponent) {
TiffComponent* tc = nullptr;
if (hasNext_) {
tc = tiffComponent.release();
pNext_ = tc;
pNext_ = std::move(tiffComponent);
return pNext_.get();
}
return tc;
return nullptr;
} // TiffDirectory::doAddNext

TiffComponent* TiffMnEntry::doAddNext(TiffComponent::UniquePtr tiffComponent) {
Expand Down Expand Up @@ -876,7 +855,7 @@ size_t TiffDirectory::doWrite(IoWrapper& ioWrapper, ByteOrder byteOrder, size_t
idx += 2;
// b) Directory entries - may contain pointers to the value or data
for (auto&& component : components_) {
idx += writeDirEntry(ioWrapper, byteOrder, offset, component, valueIdx, dataIdx, imageIdx);
idx += writeDirEntry(ioWrapper, byteOrder, offset, component.get(), valueIdx, dataIdx, imageIdx);
if (size_t sv = component->size(); sv > 4) {
sv += sv & 1; // Align value to word boundary
valueIdx += sv;
Expand Down Expand Up @@ -1205,15 +1184,15 @@ size_t TiffComponent::writeImage(IoWrapper& ioWrapper, ByteOrder byteOrder) cons
size_t TiffDirectory::doWriteImage(IoWrapper& ioWrapper, ByteOrder byteOrder) const {
size_t len = 0;
TiffComponent* pSubIfd = nullptr;
for (auto component : components_) {
for (const auto& component : components_) {
if (component->tag() == 0x014a) {
// Hack: delay writing of sub-IFD image data to get the order correct
#ifndef SUPPRESS_WARNINGS
if (pSubIfd) {
EXV_ERROR << "Multiple sub-IFD image data tags found\n";
}
#endif
pSubIfd = component;
pSubIfd = component.get();
continue;
}
len += component->writeImage(ioWrapper, byteOrder);
Expand Down Expand Up @@ -1502,13 +1481,13 @@ TiffType toTiffType(TypeId typeId) {
return static_cast<uint16_t>(typeId);
}

bool cmpTagLt(const TiffComponent* lhs, const TiffComponent* rhs) {
bool cmpTagLt(const std::unique_ptr<TiffComponent>& lhs, const std::unique_ptr<TiffComponent>& rhs) {
if (lhs->tag() != rhs->tag())
return lhs->tag() < rhs->tag();
return lhs->idx() < rhs->idx();
}

bool cmpGroupLt(const TiffComponent* lhs, const TiffComponent* rhs) {
bool cmpGroupLt(const std::unique_ptr<TiffDirectory>& lhs, const std::unique_ptr<TiffDirectory>& rhs) {
return lhs->group() < rhs->group();
}

Expand Down
30 changes: 15 additions & 15 deletions src/tiffcomposite_int.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ class TiffComponent {
//! TiffComponent auto_ptr type
using UniquePtr = std::unique_ptr<TiffComponent>;
//! Container type to hold all metadata
using Components = std::vector<TiffComponent*>;
using Components = std::vector<UniquePtr>;

//! @name Creators
//@{
Expand Down Expand Up @@ -390,7 +390,7 @@ class TiffEntryBase : public TiffComponent {
}

//! Virtual destructor.
~TiffEntryBase() override;
~TiffEntryBase() override = default;
//@}

//! @name NOT implemented
Expand Down Expand Up @@ -475,7 +475,7 @@ class TiffEntryBase : public TiffComponent {
}
//! Return a const pointer to the converted value of this component
[[nodiscard]] const Value* pValue() const {
return pValue_;
return pValue_.get();
}
//@}

Expand Down Expand Up @@ -552,8 +552,8 @@ class TiffEntryBase : public TiffComponent {
// storage_ DataBuf below.
byte* pData_{}; //!< Pointer to the data area

int idx_{}; //!< Unique id of the entry in the image
Value* pValue_{}; //!< Converted data value
int idx_{}; //!< Unique id of the entry in the image
std::unique_ptr<Value> pValue_; //!< Converted data value

// This DataBuf is only used when TiffEntryBase::setData is called.
// Otherwise, it remains empty. It is wrapped in a shared_ptr because
Expand Down Expand Up @@ -830,7 +830,7 @@ class TiffDirectory : public TiffComponent {
//! Default constructor
TiffDirectory(uint16_t tag, IfdId group, bool hasNext = true);
//! Virtual destructor
~TiffDirectory() override;
~TiffDirectory() override = default;
//@}

//! @name NOT implemented
Expand Down Expand Up @@ -917,9 +917,9 @@ class TiffDirectory : public TiffComponent {
//@}

// DATA
Components components_; //!< List of components in this directory
bool hasNext_; //!< True if the directory has a next pointer
TiffComponent* pNext_{}; //!< Pointer to the next IFD
Components components_; //!< List of components in this directory
bool hasNext_; //!< True if the directory has a next pointer
UniquePtr pNext_; //!< Pointer to the next IFD
};

/*!
Expand All @@ -938,7 +938,7 @@ class TiffSubIfd : public TiffEntryBase {
//! Default constructor
TiffSubIfd(uint16_t tag, IfdId group, IfdId newGroup);
//! Virtual destructor
~TiffSubIfd() override;
~TiffSubIfd() override = default;
//@}

//! @name Protected Creators
Expand Down Expand Up @@ -989,7 +989,7 @@ class TiffSubIfd : public TiffEntryBase {

private:
//! A collection of TIFF directories (IFDs)
using Ifds = std::vector<TiffDirectory*>;
using Ifds = std::vector<std::unique_ptr<TiffDirectory>>;

// DATA
IfdId newGroup_; //!< Start of the range of group numbers for the sub-IFDs
Expand Down Expand Up @@ -1073,7 +1073,7 @@ class TiffIfdMakernote : public TiffComponent {
//! Default constructor
TiffIfdMakernote(uint16_t tag, IfdId group, IfdId mnGroup, std::unique_ptr<MnHeader> pHeader, bool hasNext = true);
//! Virtual destructor
~TiffIfdMakernote() override = default;
~TiffIfdMakernote() override;
//@}

/*!
Expand Down Expand Up @@ -1269,7 +1269,7 @@ class TiffBinaryArray : public TiffEntryBase {
//! Constructor for a complex binary array
TiffBinaryArray(uint16_t tag, IfdId group, const ArraySet* arraySet, size_t setSize, CfgSelFct cfgSelFct);
//! Virtual destructor
~TiffBinaryArray() override;
~TiffBinaryArray() override = default;
TiffBinaryArray& operator=(const TiffBinaryArray&) = delete;
//@}

Expand Down Expand Up @@ -1470,13 +1470,13 @@ class TiffBinaryElement : public TiffEntryBase {
@brief Compare two TIFF component pointers by tag. Return true if the tag
of component lhs is less than that of rhs.
*/
bool cmpTagLt(const TiffComponent* lhs, const TiffComponent* rhs);
bool cmpTagLt(const TiffComponent::UniquePtr& lhs, const TiffComponent::UniquePtr& rhs);

/*!
@brief Compare two TIFF component pointers by group. Return true if the
group of component lhs is less than that of rhs.
*/
bool cmpGroupLt(const TiffComponent* lhs, const TiffComponent* rhs);
bool cmpGroupLt(const std::unique_ptr<TiffDirectory>& lhs, const std::unique_ptr<TiffDirectory>& rhs);

//! Function to create and initialize a new TIFF entry
TiffComponent::UniquePtr newTiffEntry(uint16_t tag, IfdId group);
Expand Down
27 changes: 11 additions & 16 deletions src/tiffvisitor_int.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,7 @@ void TiffDecoder::decodeIptc(const TiffEntryBase* object) {
return;
}
#ifndef SUPPRESS_WARNINGS
EXV_WARNING << "Failed to decode IPTC block found in "
<< "Directory Image, entry 0x83bb\n";
EXV_WARNING << "Failed to decode IPTC block found in " << "Directory Image, entry 0x83bb\n";

#endif
}
Expand All @@ -324,8 +323,7 @@ void TiffDecoder::decodeIptc(const TiffEntryBase* object) {
return;
}
#ifndef SUPPRESS_WARNINGS
EXV_WARNING << "Failed to decode IPTC block found in "
<< "Directory Image, entry 0x8649\n";
EXV_WARNING << "Failed to decode IPTC block found in " << "Directory Image, entry 0x8649\n";

#endif
}
Expand Down Expand Up @@ -580,8 +578,8 @@ void TiffEncoder::visitDirectory(TiffDirectory* /*object*/) {
void TiffEncoder::visitDirectoryNext(TiffDirectory* object) {
// Update type and count in IFD entries, in case they changed
byte* p = object->start() + 2;
for (auto component : object->components_) {
p += updateDirEntry(p, byteOrder(), component);
for (const auto& component : object->components_) {
p += updateDirEntry(p, byteOrder(), component.get());
}
}

Expand Down Expand Up @@ -1229,8 +1227,7 @@ void TiffReader::readTiffEntry(TiffEntryBase* object) {
if (p + 12 > pLast_) {
#ifndef SUPPRESS_WARNINGS
EXV_ERROR << "Entry in directory " << groupName(object->group())
<< "requests access to memory beyond the data buffer. "
<< "Skipping entry.\n";
<< "requests access to memory beyond the data buffer. " << "Skipping entry.\n";
#endif
return;
}
Expand Down Expand Up @@ -1284,9 +1281,8 @@ void TiffReader::readTiffEntry(TiffEntryBase* object) {
} else {
#ifndef SUPPRESS_WARNINGS
EXV_ERROR << "Offset of directory " << groupName(object->group()) << ", entry 0x" << std::setw(4)
<< std::setfill('0') << std::hex << object->tag() << " is out of bounds: "
<< "Offset = 0x" << std::setw(8) << std::setfill('0') << std::hex << offset
<< "; truncating the entry\n";
<< std::setfill('0') << std::hex << object->tag() << " is out of bounds: " << "Offset = 0x"
<< std::setw(8) << std::setfill('0') << std::hex << offset << "; truncating the entry\n";
#endif
}
size = 0;
Expand All @@ -1302,11 +1298,10 @@ void TiffReader::readTiffEntry(TiffEntryBase* object) {
// check for size being invalid
if (size > static_cast<size_t>(pLast_ - pData)) {
#ifndef SUPPRESS_WARNINGS
EXV_ERROR << "Upper boundary of data for "
<< "directory " << groupName(object->group()) << ", entry 0x" << std::setw(4) << std::setfill('0')
<< std::hex << object->tag() << " is out of bounds: "
<< "Offset = 0x" << std::setw(8) << std::setfill('0') << std::hex << offset << ", size = " << std::dec
<< size
EXV_ERROR << "Upper boundary of data for " << "directory " << groupName(object->group()) << ", entry 0x"
<< std::setw(4) << std::setfill('0') << std::hex << object->tag()
<< " is out of bounds: " << "Offset = 0x" << std::setw(8) << std::setfill('0') << std::hex << offset
<< ", size = " << std::dec << size
<< ", exceeds buffer size by "
// cast to make MSVC happy
<< size - static_cast<size_t>(pLast_ - pData) << " Bytes; truncating the entry\n";
Expand Down

0 comments on commit 0098d71

Please sign in to comment.