got rid of duplicated file/directory existence implementations / improved errorhandling and testing of `FileLister` (#5350)

Makefile

@@ -737,7 +737,7 @@ test/testerrorlogger.o: test/testerrorlogger.cpp externals/tinyxml2/tinyxml2.h l
 test/testexceptionsafety.o: test/testexceptionsafety.cpp lib/check.h lib/checkexceptionsafety.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testexceptionsafety.cpp
 
-test/testfilelister.o: test/testfilelister.cpp cli/filelister.h lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/pathmatch.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h test/fixture.h
+test/testfilelister.o: test/testfilelister.cpp cli/filelister.h lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/pathmatch.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h test/fixture.h
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testfilelister.cpp
 
 test/testfunctions.o: test/testfunctions.cpp lib/check.h lib/checkfunctions.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h
@@ -779,7 +779,7 @@ test/testoptions.o: test/testoptions.cpp lib/check.h lib/color.h lib/config.h li
 test/testother.o: test/testother.cpp externals/simplecpp/simplecpp.h lib/check.h lib/checkother.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/preprocessor.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testother.cpp
 
-test/testpath.o: test/testpath.cpp lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h test/fixture.h
+test/testpath.o: test/testpath.cpp lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h test/helpers.h
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testpath.cpp
 
 test/testpathmatch.o: test/testpathmatch.cpp lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/pathmatch.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h test/fixture.h

cli/cmdlineparser.cpp

@@ -285,7 +285,7 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
                 if (endsWith(mSettings.buildDir, '/'))
                     mSettings.buildDir.pop_back();
 
-                if (!Path::directoryExists(mSettings.buildDir)) {
+                if (!Path::isDirectory(mSettings.buildDir)) {
                     printError("Directory '" + mSettings.buildDir + "' specified by --cppcheck-build-dir argument has to be existent.");
                     return false;
                 }
@@ -452,7 +452,7 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
                     path = Path::fromNativeSeparators(path);
                     path = Path::simplifyPath(path);
 
-                    if (FileLister::isDirectory(path)) {
+                    if (Path::isDirectory(path)) {
                         // If directory name doesn't end with / or \, add it
                         if (!endsWith(path, '/'))
                             path += '/';
@@ -651,7 +651,7 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
                     mSettings.plistOutput += '/';
 
                 const std::string plistOutput = Path::toNativeSeparators(mSettings.plistOutput);
-                if (!FileLister::isDirectory(plistOutput)) {
+                if (!Path::isDirectory(plistOutput)) {
                     std::string message("plist folder does not exist: \"");
                     message += plistOutput;
                     message += "\".";

cli/cppcheckexecutor.cpp

@@ -137,7 +137,7 @@ bool CppCheckExecutor::parseFromArgs(Settings &settings, int argc, const char* c
              iter != settings.includePaths.end();
              ) {
             const std::string path(Path::toNativeSeparators(*iter));
-            if (FileLister::isDirectory(path))
+            if (Path::isDirectory(path))
                 ++iter;
             else {
                 // If the include path is not found, warn user and remove the non-existing path from the list.

cli/filelister.cpp

@@ -27,6 +27,7 @@
 #include <cstring>
 // fix NAME_MAX not found on macOS GCC8.1
 #include <climits>
+#include <memory>
 
 #ifdef _WIN32
 
@@ -41,23 +42,6 @@
 // When compiling Unicode targets WinAPI automatically uses *W Unicode versions
 // of called functions. Thus, we explicitly call *A versions of the functions.
 
-static BOOL myIsDirectory(const std::string& path)
-{
-// See http://msdn.microsoft.com/en-us/library/bb773621(VS.85).aspx
-    return PathIsDirectoryA(path.c_str());
-}
-
-static HANDLE myFindFirstFile(const std::string& path, LPWIN32_FIND_DATAA findData)
-{
-    HANDLE hFind = FindFirstFileA(path.c_str(), findData);
-    return hFind;
-}
-
-static BOOL myFileExists(const std::string& path)
-{
-    return PathFileExistsA(path.c_str()) && !PathIsDirectoryA(path.c_str());
-}
-
 std::string FileLister::recursiveAddFiles(std::map<std::string, std::size_t> &files, const std::string &path, const std::set<std::string> &extra, const PathMatch& ignored)
 {
     return addFiles(files, path, extra, true, ignored);
@@ -65,6 +49,9 @@ std::string FileLister::recursiveAddFiles(std::map<std::string, std::size_t> &fi
 
 std::string FileLister::addFiles(std::map<std::string, std::size_t> &files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored)
 {
+    if (path.empty())
+        return "no path specified";
+
     const std::string cleanedPath = Path::toNativeSeparators(path);
 
     // basedir is the base directory which is used to form pathnames.
@@ -75,7 +62,7 @@ std::string FileLister::addFiles(std::map<std::string, std::size_t> &files, cons
     std::string searchPattern = cleanedPath;
 
     // The user wants to check all files in a dir
-    const bool checkAllFilesInDir = (myIsDirectory(cleanedPath) != FALSE);
+    const bool checkAllFilesInDir = Path::isDirectory(cleanedPath);
 
     if (checkAllFilesInDir) {
         const char c = cleanedPath.back();
@@ -100,14 +87,20 @@ std::string FileLister::addFiles(std::map<std::string, std::size_t> &files, cons
     }
 
     WIN32_FIND_DATAA ffd;
-    HANDLE hFind = myFindFirstFile(searchPattern, &ffd);
+    HANDLE hFind = FindFirstFileA(searchPattern.c_str(), &ffd);
-    if (INVALID_HANDLE_VALUE == hFind)
+    if (INVALID_HANDLE_VALUE == hFind) {
+        const DWORD err = GetLastError();
+        if (err == ERROR_FILE_NOT_FOUND) {
+            // no files matched
             return "";
+        }
+        return "finding files failed (error: " + std::to_string(err) + ")";
+    }
+    std::unique_ptr<void, decltype(&FindClose)> hFind_deleter(hFind, FindClose);
 
     do {
-        if (ffd.cFileName[0] == '.' || ffd.cFileName[0] == '\0')
+        if (ffd.cFileName[0] != '.' && ffd.cFileName[0] != '\0')
-            continue;
+        {
-
             const char* ansiFfd = ffd.cFileName;
             if (std::strchr(ansiFfd,'?')) {
                 ansiFfd = ffd.cAlternateFileName;
@@ -137,23 +130,20 @@ std::string FileLister::addFiles(std::map<std::string, std::size_t> &files, cons
                     }
                 }
             }
-    } while (FindNextFileA(hFind, &ffd) != FALSE);
+        }
+
+        if (!FindNextFileA(hFind, &ffd)) {
+            const DWORD err = GetLastError();
+            // no more files matched
+            if (err != ERROR_NO_MORE_FILES)
+                return "failed to get next file (error: " + std::to_string(err) + ")";
+            break;
+        }
+    } while (true);
 
-    FindClose(hFind);
     return "";
 }
 
-bool FileLister::isDirectory(const std::string &path)
-{
-    return (myIsDirectory(path) != FALSE);
-}
-
-bool FileLister::fileExists(const std::string &path)
-{
-    return (myFileExists(path) != FALSE);
-}
-
-
 #else
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -189,8 +179,11 @@ static std::string addFiles2(std::map<std::string, std::size_t> &files,
     if (stat(path.c_str(), &file_stat) != -1) {
         if ((file_stat.st_mode & S_IFMT) == S_IFDIR) {
             DIR * dir = opendir(path.c_str());
-            if (!dir)
+            if (!dir) {
-                return "";
+                const int err = errno;
+                return "could not open directory '" + path + "' (errno: " + std::to_string(err) + ")";
+            }
+            std::unique_ptr<DIR, decltype(&closedir)> dir_deleter(dir, closedir);
 
             std::string new_path = path;
             new_path += '/';
@@ -204,15 +197,14 @@ static std::string addFiles2(std::map<std::string, std::size_t> &files,
                 new_path += dir_result->d_name;
 
 #if defined(_DIRENT_HAVE_D_TYPE) || defined(_BSD_SOURCE)
-                const bool path_is_directory = (dir_result->d_type == DT_DIR || (dir_result->d_type == DT_UNKNOWN && FileLister::isDirectory(new_path)));
+                const bool path_is_directory = (dir_result->d_type == DT_DIR || (dir_result->d_type == DT_UNKNOWN && Path::isDirectory(new_path)));
 #else
-                const bool path_is_directory = FileLister::isDirectory(new_path);
+                const bool path_is_directory = Path::isDirectory(new_path);
 #endif
                 if (path_is_directory) {
                     if (recursive && !ignored.match(new_path)) {
                         std::string err = addFiles2(files, new_path, extra, recursive, ignored);
                         if (!err.empty()) {
-                            closedir(dir);
                             return err;
                         }
                     }
@@ -221,13 +213,12 @@ static std::string addFiles2(std::map<std::string, std::size_t> &files,
                         if (stat(new_path.c_str(), &file_stat) != -1)
                             files[new_path] = file_stat.st_size;
                         else {
-                            closedir(dir);
+                            const int err = errno;
-                            return "Can't stat " + new_path + " errno: " + std::to_string(errno);
+                            return "could not stat file '" + new_path + "' (errno: " + std::to_string(err) + ")";
                         }
                     }
                 }
             }
-            closedir(dir);
         } else
             files[path] = file_stat.st_size;
     }
@@ -241,27 +232,14 @@ std::string FileLister::recursiveAddFiles(std::map<std::string, std::size_t> &fi
 
 std::string FileLister::addFiles(std::map<std::string, std::size_t> &files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored)
 {
-    if (!path.empty()) {
+    if (path.empty())
+        return "no path specified";
+
     std::string corrected_path = path;
     if (endsWith(corrected_path, '/'))
         corrected_path.erase(corrected_path.end() - 1);
 
     return addFiles2(files, corrected_path, extra, recursive, ignored);
-    }
-
-    return "";
-}
-
-bool FileLister::isDirectory(const std::string &path)
-{
-    struct stat file_stat;
-    return (stat(path.c_str(), &file_stat) != -1 && (file_stat.st_mode & S_IFMT) == S_IFDIR);
-}
-
-bool FileLister::fileExists(const std::string &path)
-{
-    struct stat file_stat;
-    return (stat(path.c_str(), &file_stat) != -1 && (file_stat.st_mode & S_IFMT) == S_IFREG);
 }
 
 #endif

cli/filelister.h

@@ -73,18 +73,6 @@ public:
      * @return On success, an empty string is returned. On error, a error message is returned.
      */
     static std::string addFiles(std::map<std::string, std::size_t> &files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored);
-
-    /**
-     * @brief Is given path a directory?
-     * @return returns true if the path is a directory
-     */
-    static bool isDirectory(const std::string &path);
-
-    /**
-     * @brief Check if the given path is a file and if it exists?
-     * @return true if path points to file and the file exists.
-     */
-    static bool fileExists(const std::string &path);
 };
 
 /// @}

lib/cppcheck.cpp

@@ -99,19 +99,19 @@ namespace {
         std::string runScript;
 
         static std::string getFullPath(const std::string &fileName, const std::string &exename) {
-            if (Path::fileExists(fileName))
+            if (Path::isFile(fileName))
                 return fileName;
 
             const std::string exepath = Path::getPathFromFilename(exename);
-            if (Path::fileExists(exepath + fileName))
+            if (Path::isFile(exepath + fileName))
                 return exepath + fileName;
-            if (Path::fileExists(exepath + "addons/" + fileName))
+            if (Path::isFile(exepath + "addons/" + fileName))
                 return exepath + "addons/" + fileName;
 
 #ifdef FILESDIR
-            if (Path::fileExists(FILESDIR + ("/" + fileName)))
+            if (Path::isFile(FILESDIR + ("/" + fileName)))
                 return FILESDIR + ("/" + fileName);
-            if (Path::fileExists(FILESDIR + ("/addons/" + fileName)))
+            if (Path::isFile(FILESDIR + ("/addons/" + fileName)))
                 return FILESDIR + ("/addons/" + fileName);
 #endif
             return "";

lib/importproject.cpp

@@ -1367,5 +1367,5 @@ void ImportProject::printError(const std::string &message)
 
 bool ImportProject::sourceFileExists(const std::string &file)
 {
-    return Path::fileExists(file);
+    return Path::isFile(file);
 }

lib/path.cpp

@@ -261,17 +261,26 @@ std::string Path::stripDirectoryPart(const std::string &file)
     return file;
 }
 
-bool Path::fileExists(const std::string &file)
+#ifdef _WIN32
+using mode_t = unsigned short;
+#endif
+
+static mode_t file_type(const std::string &path)
 {
-    std::ifstream f(file.c_str());
+    struct stat file_stat;
-    return f.is_open();
+    if (stat(path.c_str(), &file_stat) == -1)
+        return 0;
+    return file_stat.st_mode & S_IFMT;
 }
 
-
+bool Path::isFile(const std::string &path)
-bool Path::directoryExists(const std::string &path)
 {
-    struct stat info;
+    return file_type(path) == S_IFREG;
-    return stat(path.c_str(), &info) == 0 && (info.st_mode & S_IFDIR);
+}
+
+bool Path::isDirectory(const std::string &path)
+{
+    return file_type(path) == S_IFDIR;
 }
 
 std::string Path::join(std::string path1, std::string path2) {

lib/path.h

@@ -181,18 +181,18 @@ public:
     static std::string stripDirectoryPart(const std::string &file);
 
     /**
-     * @brief Checks if a File exists
+     * @brief Checks if given path is a file
-     * @param file Path to be checked if it is a File
+     * @param path Path to be checked
-     * @return true if given path is a File
+     * @return true if given path is a file
      */
-    static bool fileExists(const std::string &file);
+    static bool isFile(const std::string &path);
 
     /**
-     * @brief Checks if a directory exists
+     * @brief Checks if a given path is a directory
      * @param path Path to be checked
      * @return true if given path is a directory
      */
-    static bool directoryExists(const std::string &path);
+    static bool isDirectory(const std::string &path);
 
     /**
      * join 2 paths with '/' separators

lib/settings.cpp

@@ -45,7 +45,7 @@ void Settings::loadCppcheckCfg()
 {
     std::string fileName = Path::getPathFromFilename(exename) + "cppcheck.cfg";
 #ifdef FILESDIR
-    if (Path::fileExists(FILESDIR "/cppcheck.cfg"))
+    if (Path::isFile(FILESDIR "/cppcheck.cfg"))
         fileName = FILESDIR "/cppcheck.cfg";
 #endif
 

test/testfilelister.cpp

@@ -17,11 +17,11 @@
  */
 
 #include "filelister.h"
+#include "path.h"
 #include "pathmatch.h"
 #include "fixture.h"
 
 #include <cstddef>
-#include <fstream>
 #include <map>
 #include <string>
 #include <vector>
@@ -33,66 +33,82 @@ public:
 
 private:
     void run() override {
-        // bail out if the tests are not executed from the base folder
-        {
-            std::ifstream fin("test/testfilelister.cpp");
-            if (!fin.is_open()) {
-                // TODO: log
-                return;
-            }
-        }
-
-        TEST_CASE(isDirectory);
         TEST_CASE(recursiveAddFiles);
-        TEST_CASE(excludeFile);
+        TEST_CASE(recursiveAddFilesEmptyPath);
-        TEST_CASE(fileExists);
+        TEST_CASE(excludeFile1);
+        TEST_CASE(excludeFile2);
     }
 
-    void isDirectory() const {
+    // TODO: generate file list instead
-        ASSERT_EQUALS(false, FileLister::isDirectory("readme.txt"));
+    static std::string findBaseDir() {
-        ASSERT_EQUALS(true, FileLister::isDirectory("lib"));
+        std::string basedir;
+        while (!Path::isDirectory(Path::join(basedir, ".github"))) {
+            basedir += "../";
+        }
+        return basedir;
     }
 
     void recursiveAddFiles() const {
+        const std::string adddir = findBaseDir() + ".";
+
         // Recursively add add files..
         std::map<std::string, std::size_t> files;
         std::vector<std::string> masks;
         PathMatch matcher(masks);
-        std::string err = FileLister::recursiveAddFiles(files, ".", matcher);
+        std::string err = FileLister::recursiveAddFiles(files, adddir, matcher);
-        ASSERT(err.empty());
+        ASSERT_EQUALS("", err);
 
-        // In case there are leading "./"..
+        ASSERT(!files.empty());
-        for (std::map<std::string, std::size_t>::iterator i = files.begin(); i != files.end();) {
+
-            if (i->first.compare(0,2,"./") == 0) {
+#ifdef _WIN32
-                files[i->first.substr(2)] = i->second;
+        std::string dirprefix;
-                i = files.erase(i);
+        if (adddir != ".")
-            } else
+            dirprefix = adddir + "/";
-                ++i;
+#else
-        }
+        const std::string dirprefix = adddir + "/";
+#endif
 
         // Make sure source files are added..
-        ASSERT(files.find("cli/main.cpp") != files.end());
+        ASSERT(files.find(dirprefix + "cli/main.cpp") != files.end());
-        ASSERT(files.find("lib/token.cpp") != files.end());
+        ASSERT(files.find(dirprefix + "lib/token.cpp") != files.end());
-        ASSERT(files.find("lib/tokenize.cpp") != files.end());
+        ASSERT(files.find(dirprefix + "lib/tokenize.cpp") != files.end());
-        ASSERT(files.find("test/testfilelister.cpp") != files.end());
+        ASSERT(files.find(dirprefix + "gui/main.cpp") != files.end());
+        ASSERT(files.find(dirprefix + "test/testfilelister.cpp") != files.end());
 
         // Make sure headers are not added..
-        ASSERT(files.find("lib/tokenize.h") == files.end());
+        ASSERT(files.find(dirprefix + "lib/tokenize.h") == files.end());
     }
 
-    void excludeFile() const {
+    void recursiveAddFilesEmptyPath() const {
+        std::map<std::string, std::size_t> files;
+        const std::string err = FileLister::recursiveAddFiles(files, "", PathMatch({}));
+        ASSERT_EQUALS("no path specified", err);
+    }
+
+    void excludeFile1() const {
+        const std::string basedir = findBaseDir();
+
         std::map<std::string, std::size_t> files;
         std::vector<std::string> ignored{"lib/token.cpp"};
         PathMatch matcher(ignored);
-        std::string err = FileLister::recursiveAddFiles(files, "lib/token.cpp", matcher);
+        std::string err = FileLister::recursiveAddFiles(files, basedir + "lib/token.cpp", matcher);
-        ASSERT(err.empty());
+        ASSERT_EQUALS("", err);
         ASSERT(files.empty());
     }
 
-    void fileExists() const {
+    void excludeFile2() const {
-        ASSERT_EQUALS(false, FileLister::fileExists("lib"));
+        const std::string basedir = findBaseDir();
-        ASSERT_EQUALS(true, FileLister::fileExists("readme.txt"));
+
+        std::map<std::string, std::size_t> files;
+        std::vector<std::string> ignored;
+        PathMatch matcher(ignored);
+        std::string err = FileLister::recursiveAddFiles(files, basedir + "lib/token.cpp", matcher);
+        ASSERT_EQUALS("", err);
+        ASSERT_EQUALS(1, files.size());
+        ASSERT_EQUALS(basedir + "lib/token.cpp", files.begin()->first);
     }
+
+    // TODO: test errors
 };
 
 REGISTER_TEST(TestFileLister)

test/testpath.cpp

@@ -18,6 +18,7 @@
 
 #include "path.h"
 #include "fixture.h"
+#include "helpers.h"
 
 #include <string>
 #include <vector>
@@ -39,6 +40,8 @@ private:
         TEST_CASE(is_cpp);
         TEST_CASE(get_path_from_filename);
         TEST_CASE(join);
+        TEST_CASE(isDirectory);
+        TEST_CASE(isFile);
     }
 
     void removeQuotationMarks() const {
@@ -155,6 +158,24 @@ private:
         ASSERT_EQUALS("a/b", Path::join("a/", "b"));
         ASSERT_EQUALS("/b", Path::join("a", "/b"));
     }
+
+    void isDirectory() const {
+        ScopedFile file("testpath.txt", "", "testpath");
+        ScopedFile file2("testpath2.txt", "");
+        ASSERT_EQUALS(false, Path::isDirectory("testpath.txt"));
+        ASSERT_EQUALS(true, Path::isDirectory("testpath"));
+        ASSERT_EQUALS(false, Path::isDirectory("testpath/testpath.txt"));
+        ASSERT_EQUALS(false, Path::isDirectory("testpath2.txt"));
+    }
+
+    void isFile() const {
+        ScopedFile file("testpath.txt", "", "testpath");
+        ScopedFile file2("testpath2.txt", "");
+        ASSERT_EQUALS(false, Path::isFile("testpath"));
+        ASSERT_EQUALS(false, Path::isFile("testpath.txt"));
+        ASSERT_EQUALS(true, Path::isFile("testpath/testpath.txt"));
+        ASSERT_EQUALS(true, Path::isFile("testpath2.txt"));
+    }
 };
 
 REGISTER_TEST(TestPath)