Skip to content

Commit

Permalink
Revisiting cmake code for generating coverage reports (#2047)
Browse files Browse the repository at this point in the history 
* cmake: better usage of gcovr for coverage reports

* Add test for FileIo::setPath

* Remove useless seek() overload

* Add missing override specifiers

* ignore .vs folder

* Small refactors in BasicIo implementations

* Remove duplicated doxygen doc

* Refactor & add tests for MemIO

* Fix compilation warnings on windows
  • Loading branch information
@piponazo
piponazo authored Jan 6, 2022
1 parent a094dde commit 8135665
Show file tree
Hide file tree
Showing 6 changed files with 163 additions and 177 deletions.
3 changes: 2 additions & 1 deletion .gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,5 @@ src/doxygen.hpp
test/tmp/*
doc/html
contrib/vms/.vagrant
/.vscode
/.vscode
.vs/
13 changes: 8 additions & 5 deletions cmake/gcovr.cmake
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# Intentd usage
# cmake -DBUILD_WITH_COVERAGE=yes ../
# make -j
# make tests
# make coverage
# Intended usage
# cmake -DCMAKE_BUILD_TYPE=Debug -DBUILD_WITH_COVERAGE=yes ../
# cmake --build . --config Debug
# ctest
# cmake --build . --config Debug --target coverage

if(BUILD_WITH_COVERAGE)
find_program (GCOVR gcovr)
Expand All @@ -12,6 +12,9 @@ if(BUILD_WITH_COVERAGE)
add_custom_command(OUTPUT _run_gcovr_parser
POST_BUILD
COMMAND ${GCOVR} --root ${CMAKE_SOURCE_DIR} --object-dir=${CMAKE_BINARY_DIR} --html --html-details -o coverage_output/coverage.html
--exclude-directories xmpsdk --exclude-directories unitTests --exclude-directories samples
--exclude '.*xmpsdk.*' --exclude '.*unitTests.*' --exclude '.*samples.*'

WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
)
add_custom_target (coverage DEPENDS _run_gcovr_parser)
Expand Down
57 changes: 10 additions & 47 deletions include/exiv2/basicio.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,11 +183,8 @@ namespace Exiv2 {
@return 0 if successful;<BR>
Nonzero if failure;
*/
#if defined(_MSC_VER)
virtual int seek(int64_t offset, Position pos) = 0;
#else
virtual int seek(long offset, Position pos) = 0;
#endif

/*!
@brief Safe version of `seek()` that checks for errors and throws
an exception if the seek was unsuccessful.
Expand All @@ -196,11 +193,7 @@ namespace Exiv2 {
@param pos Position from which the seek should start
@param err Error code to use if an exception is thrown.
*/
#if defined(_MSC_VER)
void seekOrThrow(int64_t offset, Position pos, ErrorCode err);
#else
void seekOrThrow(long offset, Position pos, ErrorCode err);
#endif

/*!
@brief Direct access to the IO data. For files, this is done by
Expand Down Expand Up @@ -263,7 +256,7 @@ namespace Exiv2 {
@note This method should be only called after the concerned data (metadata)
are all downloaded from the remote file to memory.
*/
virtual void populateFakeData() {}
virtual void populateFakeData() = 0;

/*!
@brief this is allocated and populated by mmap()
Expand Down Expand Up @@ -442,19 +435,9 @@ namespace Exiv2 {
@throw Error In case of failure
*/
void transfer(BasicIo& src) override;
/*!
@brief Move the current file position.
@param offset Number of bytes to move the file position
relative to the starting position specified by \em pos
@param pos Position from which the seek should start
@return 0 if successful;<BR>
Nonzero if failure;
*/
#if defined(_MSC_VER)

int seek(int64_t offset, Position pos) override;
#else
int seek(long offset, Position pos) override;
#endif

/*!
@brief Map the file into the process's address space. The file must be
open before mmap() is called. If the mapped area is writeable,
Expand Down Expand Up @@ -664,19 +647,9 @@ namespace Exiv2 {
@throw Error In case of failure
*/
void transfer(BasicIo& src) override;
/*!
@brief Move the current IO position.
@param offset Number of bytes to move the IO position
relative to the starting position specified by \em pos
@param pos Position from which the seek should start
@return 0 if successful;<BR>
Nonzero if failure;
*/
#if defined(_MSC_VER)
virtual int seek(int64_t offset, Position pos);
#else
int seek(long offset, Position pos) override;
#endif

int seek(int64_t offset, Position pos) override;

/*!
@brief Allow direct access to the underlying data buffer. The buffer
is not protected against write access in any way, the argument
Expand Down Expand Up @@ -949,19 +922,9 @@ namespace Exiv2 {
@note The write access is only supported by http, https, ssh.
*/
void transfer(BasicIo& src) override;
/*!
@brief Move the current IO position.
@param offset Number of bytes to move the IO position
relative to the starting position specified by \em pos
@param pos Position from which the seek should start
@return 0 if successful;<BR>
Nonzero if failure;
*/
#if defined(_MSC_VER)
virtual int seek(int64_t offset, Position pos);
#else
int seek(long offset, Position pos) override;
#endif

int seek(int64_t offset, Position pos) override;

/*!
@brief Not support
@return NULL
Expand Down
85 changes: 9 additions & 76 deletions src/basicio.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,7 @@ namespace Exiv2 {
enforce(!error(), err);
}

#if defined(_MSC_VER)
void BasicIo::seekOrThrow(int64_t offset, Position pos, ErrorCode err) {
#else
void BasicIo::seekOrThrow(long offset, Position pos, ErrorCode err) {
#endif
const int r = seek(offset, pos);
enforce(r == 0, err);
}
Expand Down Expand Up @@ -628,6 +624,7 @@ namespace Exiv2 {
fileIo->close();
// Check if the file can be written to, if it already exists
if (open("a+b") != 0) {
/// \todo Use std::filesystem once C++17 can be used
// Remove the (temporary) file
#ifdef EXV_UNICODE_PATH
if (fileIo->p_->wpMode_ == Impl::wpUnicode) {
Expand Down Expand Up @@ -898,7 +895,6 @@ namespace Exiv2 {
return putc(data, p_->fp_);
}

#if defined(_MSC_VER)
int FileIo::seek( int64_t offset, Position pos )
{
assert(p_->fp_ != 0);
Expand All @@ -917,24 +913,6 @@ namespace Exiv2 {
return std::fseek(p_->fp_,static_cast<long>(offset), fileSeek);
#endif
}
#else
int FileIo::seek(long offset, Position pos)
{
assert(p_->fp_ != 0);

int fileSeek = 0;
switch (pos) {
case BasicIo::cur: fileSeek = SEEK_CUR; break;
case BasicIo::beg: fileSeek = SEEK_SET; break;
case BasicIo::end: fileSeek = SEEK_END; break;
}

if (p_->switchMode(Impl::opSeek) != 0) {
return 1;
}
return std::fseek(p_->fp_, offset, fileSeek);
}
#endif

long FileIo::tell() const
{
Expand Down Expand Up @@ -1279,7 +1257,6 @@ namespace Exiv2 {
return data;
}

#if defined(_MSC_VER)
int MemIo::seek( int64_t offset, Position pos )
{
int64_t newIdx = 0;
Expand All @@ -1302,30 +1279,6 @@ namespace Exiv2 {
p_->eof_ = false;
return 0;
}
#else
int MemIo::seek(long offset, Position pos)
{
long newIdx = 0;

switch (pos) {
case BasicIo::cur: newIdx = p_->idx_ + offset; break;
case BasicIo::beg: newIdx = offset; break;
case BasicIo::end: newIdx = p_->size_ + offset; break;
}

if (newIdx < 0)
return 1;

if (newIdx > p_->size_) {
p_->eof_ = true;
return 1;
}

p_->idx_ = newIdx;
p_->eof_ = false;
return 0;
}
#endif

byte* MemIo::mmap(bool /*isWriteable*/)
{
Expand Down Expand Up @@ -1368,22 +1321,21 @@ namespace Exiv2 {
{
DataBuf buf(rcount);
long readCount = read(buf.data(), buf.size());
if (readCount < 0) {
throw Error(kerInputDataReadFailed);
}
buf.resize(readCount);
return buf;
}

long MemIo::read(byte* buf, long rcount)
{
long avail = std::max(p_->size_ - p_->idx_, 0L);
long allow = std::min(rcount, avail);
const long avail = std::max(p_->size_ - p_->idx_, 0L);
const long allow = std::min(rcount, avail);
if (allow > 0) {
std::memcpy(buf, &p_->data_[p_->idx_], allow);
}
p_->idx_ += allow;
if (rcount > avail) p_->eof_ = true;
if (rcount > avail) {
p_->eof_ = true;
}
return allow;
}

Expand Down Expand Up @@ -1919,28 +1871,10 @@ namespace Exiv2 {
src.close();
}

#if defined(_MSC_VER)
int RemoteIo::seek( int64_t offset, Position pos )
int RemoteIo::seek( int64_t offset, Position pos)
{
assert(p_->isMalloced_);
uint64_t newIdx = 0;

switch (pos) {
case BasicIo::cur: newIdx = p_->idx_ + offset; break;
case BasicIo::beg: newIdx = offset; break;
case BasicIo::end: newIdx = p_->size_ + offset; break;
}

if ( /*newIdx < 0 || */ newIdx > static_cast<uint64_t>(p_->size_) ) return 1;
p_->idx_ = static_cast<long>(newIdx); //not very sure about this. need more test!! - note by Shawn fly2xj@gmail.com //TODO
p_->eof_ = false;
return 0;
}
#else
int RemoteIo::seek(long offset, Position pos)
{
assert(p_->isMalloced_);
long newIdx = 0;
int64_t newIdx = 0;

switch (pos) {
case BasicIo::cur: newIdx = p_->idx_ + offset; break;
Expand All @@ -1950,13 +1884,12 @@ namespace Exiv2 {

// #1198. Don't return 1 when asked to seek past EOF. Stay calm and set eof_
// if (newIdx < 0 || newIdx > (long) p_->size_) return 1;
p_->idx_ = newIdx;
p_->idx_ = static_cast<long>(newIdx);
p_->eof_ = newIdx > static_cast<long>(p_->size_);
if (p_->idx_ > static_cast<long>(p_->size_))
p_->idx_ = static_cast<long>(p_->size_);
return 0;
}
#endif

byte* RemoteIo::mmap(bool /*isWriteable*/)
{
Expand Down
20 changes: 19 additions & 1 deletion unitTests/test_FileIo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ namespace
{
const std::string testData(TESTDATA_PATH);
const std::string imagePath(testData + "/DSC_3079.jpg");
const std::string nonExistingImagePath(testData + "/nonExisting.jpg");
} // namespace

TEST(AFileIO, canBeInstantiatedWithFilePath)
Expand All @@ -43,10 +44,27 @@ TEST(AFileIO, isOpenDoItsJob)
{
FileIo file(imagePath);
ASSERT_FALSE(file.isopen());
file.open();
ASSERT_EQ(0, file.open());
ASSERT_TRUE(file.isopen());
}

TEST(AFileIO, failsToOpenANonExistingFile)
{
FileIo file(nonExistingImagePath);
ASSERT_FALSE(file.isopen());
ASSERT_EQ(1, file.open());
ASSERT_FALSE(file.isopen());
}

TEST(AFileIO, canChangeItsPathWithSetPath)
{
FileIo file(nonExistingImagePath);
ASSERT_EQ(nonExistingImagePath, file.path());
file.setPath(imagePath);
ASSERT_EQ(imagePath, file.path());
}


TEST(AFileIO, returnsFileSizeIfItsOpened)
{
FileIo file(imagePath);
Expand Down
Loading

0 comments on commit 8135665

Please sign in to comment.