Addressed code review comments.

coderye · Feb 25, 2016 · 246907d · 246907d
1 parent 5d6a468
commit 246907d
Show file tree

Hide file tree

Showing 7 changed files with 36 additions and 28 deletions.
diff --git a/Makefile b/Makefile
@@ -457,12 +457,12 @@ ifdef LIBZIP_PATH
 endif
 
 IMAGEREADER_SRC =\
-	$(SOURCEDIR)/Readers/ImageReader/Exports.cpp \
-	$(SOURCEDIR)/Readers/ImageReader/ImageConfigHelper.cpp \
-	$(SOURCEDIR)/Readers/ImageReader/ImageDataDeserializer.cpp \
-	$(SOURCEDIR)/Readers/ImageReader/ImageTransformers.cpp \
-	$(SOURCEDIR)/Readers/ImageReader/ImageReader.cpp \
-        $(SOURCEDIR)/Readers/ImageReader/ZipByteReader.cpp \
+  $(SOURCEDIR)/Readers/ImageReader/Exports.cpp \
+  $(SOURCEDIR)/Readers/ImageReader/ImageConfigHelper.cpp \
+  $(SOURCEDIR)/Readers/ImageReader/ImageDataDeserializer.cpp \
+  $(SOURCEDIR)/Readers/ImageReader/ImageTransformers.cpp \
+  $(SOURCEDIR)/Readers/ImageReader/ImageReader.cpp \
+  $(SOURCEDIR)/Readers/ImageReader/ZipByteReader.cpp \
 
 IMAGEREADER_OBJ := $(patsubst %.cpp, $(OBJDIR)/%.o, $(IMAGEREADER_SRC))
 

diff --git a/Source/Readers/ImageReader/ByteReader.h b/Source/Readers/ImageReader/ByteReader.h
@@ -24,10 +24,7 @@ class ByteReader
     virtual void Register(size_t seqId, const std::string& path) = 0;
     virtual cv::Mat Read(size_t seqId, const std::string& path) = 0;
 
-    ByteReader(const ByteReader&) = delete;
-    ByteReader& operator=(const ByteReader&) = delete;
-    ByteReader(ByteReader&&) = delete;
-    ByteReader& operator=(ByteReader&&) = delete;
+    DISABLE_COPY_AND_MOVE(ByteReader);
 };
 
 class FileByteReader : public ByteReader

diff --git a/Source/Readers/ImageReader/ImageDataDeserializer.cpp b/Source/Readers/ImageReader/ImageDataDeserializer.cpp
@@ -144,12 +144,11 @@ void ImageDataDeserializer::CreateSequenceDescriptions(std::string mapPath, size
         RuntimeError("Could not open %s for reading.", mapPath.c_str());
     }
 
-    PathReaderMap knownReaders;
-
     std::string line;
     ImageSequenceDescription description;
     description.m_numberOfSamples = 1;
     description.m_isValid = true;
+    PathReaderMap knownReaders;
     for (size_t lineIndex = 0; std::getline(mapFile, line); ++lineIndex)
     {
         std::stringstream ss(line);

diff --git a/Source/Readers/ImageReader/ImageDataDeserializer.h b/Source/Readers/ImageReader/ImageDataDeserializer.h
@@ -55,12 +55,12 @@ class ImageDataDeserializer : public DataDeserializerBase
     // Element type of the feature/label stream (currently float/double only).
     ElementType m_featureElementType;
 
-private:
     // Not using nocase_compare here as it's not correct on Linux.
     using PathReaderMap = std::unordered_map<std::string, std::shared_ptr<ByteReader>>;
     void RegisterByteReader(size_t seqId, const std::string& path, PathReaderMap& knownReaders);
     cv::Mat ReadImage(size_t seqId, const std::string& path);
 
+    // REVIEW alexeyk: can potentially use vector instead of map. Need to handle default reader and resizing though.
     using SeqReaderMap = std::unordered_map<size_t, std::shared_ptr<ByteReader>>;
     SeqReaderMap m_readers;
 

diff --git a/Source/Readers/ImageReader/ZipByteReader.cpp b/Source/Readers/ImageReader/ZipByteReader.cpp
@@ -70,22 +70,33 @@ cv::Mat ZipByteReader::Read(size_t seqId, const std::string& path)
     if (contents.size() < size)
         contents.resize(size);
     auto zipFile = m_zips.pop_or_create([this]() { return OpenZip(); });
-    zip_file *file = zip_fopen_index(zipFile.get(), index, 0);
-    assert(nullptr != file);
-    if (nullptr == file)
     {
-        RuntimeError("Could not open file %s in the zip file, sequence id = %lu, zip library error: %s",
-                     path.c_str(), (long)seqId, GetZipError(zip_error_code_zip(zip_get_error(zipFile.get()))).c_str());
-    }
-    assert(contents.size() >= size);
-    zip_uint64_t bytesRead = zip_fread(file, contents.data(), size);
-    assert(bytesRead == size);
-    if (bytesRead != size)
-    {
-        RuntimeError("Bytes read %lu != expected %lu while reading file %s",
-                     (long)bytesRead, (long)size, path.c_str());
+        std::unique_ptr<zip_file_t, void(*)(zip_file_t*)> file(
+            zip_fopen_index(zipFile.get(), index, 0),
+            [](zip_file_t* f)
+            {
+                assert(f != nullptr);
+                int err = zip_fclose(f);
+                assert(ZIP_ER_OK == err);
+#ifdef NDEBUG
+                UNUSED(err);
+#endif
+            });
+        assert(nullptr != file);
+        if (nullptr == file)
+        {
+            RuntimeError("Could not open file %s in the zip file, sequence id = %lu, zip library error: %s",
+                         path.c_str(), (long)seqId, GetZipError(zip_error_code_zip(zip_get_error(zipFile.get()))).c_str());
+        }
+        assert(contents.size() >= size);
+        zip_uint64_t bytesRead = zip_fread(file.get(), contents.data(), size);
+        assert(bytesRead == size);
+        if (bytesRead != size)
+        {
+            RuntimeError("Bytes read %lu != expected %lu while reading file %s",
+                         (long)bytesRead, (long)size, path.c_str());
+        }
     }
-    zip_fclose(file);
     m_zips.push(std::move(zipFile));
 
     cv::Mat img = cv::imdecode(cv::Mat(1, (int)size, CV_8UC1, contents.data()), cv::IMREAD_COLOR);

diff --git a/Tests/UnitTests/ReaderTests/ImageReaderTests.cpp b/Tests/UnitTests/ReaderTests/ImageReaderTests.cpp
@@ -54,6 +54,7 @@ BOOST_AUTO_TEST_CASE(ImageReaderZip)
 
 BOOST_AUTO_TEST_CASE(ImageReaderZipMissingFile)
 {
+    // REVIEW alexeyk: is there a way to check specific exception message?
     BOOST_CHECK_THROW(
         HelperRunReaderTest<float>(
             testDataPath() + "/Config/ImageReaderZipMissing_Config.cntk",

diff --git a/configure b/configure
@@ -482,7 +482,7 @@ do
                 then
                     echo "Cannot find libzip directory."
                     echo "Please specify a value for --with-libzip"
-                    echo "libzip can be downloaded from http://www.nih.at/libzip/"
+                    echo "libzip (v.1.1.2 or higher) can be downloaded from http://www.nih.at/libzip/"
                     exit 1
                 fi
             else