got rid of duplicated file/directory existence implementations / improved errorhandling and testing of FileLister

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,30 +42,16 @@
 // 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);
 }
 
 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,60 +87,63 @@ std::string FileLister::addFiles(std::map<std::string, std::size_t> &files, cons
     }
 
     WIN32_FIND_DATAA ffd;
-    HANDLE hFind = myFindFirstFile(searchPattern, &ffd);
-    if (INVALID_HANDLE_VALUE == hFind)
-        return "";
+    HANDLE hFind = FindFirstFileA(searchPattern.c_str(), &ffd);
+    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')
-            continue;
-
-        const char* ansiFfd = ffd.cFileName;
-        if (std::strchr(ansiFfd,'?')) {
-            ansiFfd = ffd.cAlternateFileName;
-        }
+        if (ffd.cFileName[0] != '.' && ffd.cFileName[0] != '\0')
+        {
+            const char* ansiFfd = ffd.cFileName;
+            if (std::strchr(ansiFfd,'?')) {
+                ansiFfd = ffd.cAlternateFileName;
+            }
 
-        const std::string fname(basedir + ansiFfd);
+            const std::string fname(basedir + ansiFfd);
 
-        if ((ffd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0) {
-            // File
-            if ((!checkAllFilesInDir || Path::acceptFile(fname, extra)) && !ignored.match(fname)) {
-                const std::string nativename = Path::fromNativeSeparators(fname);
+            if ((ffd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0) {
+                // File
+                if ((!checkAllFilesInDir || Path::acceptFile(fname, extra)) && !ignored.match(fname)) {
+                    const std::string nativename = Path::fromNativeSeparators(fname);
 
-                // Limitation: file sizes are assumed to fit in a 'size_t'
+                    // Limitation: file sizes are assumed to fit in a 'size_t'
 #ifdef _WIN64
-                files[nativename] = (static_cast<std::size_t>(ffd.nFileSizeHigh) << 32) | ffd.nFileSizeLow;
+                    files[nativename] = (static_cast<std::size_t>(ffd.nFileSizeHigh) << 32) | ffd.nFileSizeLow;
 #else
-                files[nativename] = ffd.nFileSizeLow;
+                    files[nativename] = ffd.nFileSizeLow;
 #endif
-            }
-        } else {
-            // Directory
-            if (recursive) {
-                if (!ignored.match(fname)) {
-                    std::string err = FileLister::recursiveAddFiles(files, fname, extra, ignored);
-                    if (!err.empty())
-                        return err;
+                }
+            } else {
+                // Directory
+                if (recursive) {
+                    if (!ignored.match(fname)) {
+                        std::string err = FileLister::recursiveAddFiles(files, fname, extra, ignored);
+                        if (!err.empty())
+                            return err;
+                    }
                 }
             }
         }
-    } while (FindNextFileA(hFind, &ffd) != FALSE);
 
-    FindClose(hFind);
-    return "";
-}
-
-bool FileLister::isDirectory(const std::string &path)
-{
-    return (myIsDirectory(path) != 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);
 
-bool FileLister::fileExists(const std::string &path)
-{
-    return (myFileExists(path) != FALSE);
+    return "";
 }
 
-
 #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)
-                return "";
+            if (!dir) {
+                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);
-                            return "Can't stat " + new_path + " errno: " + std::to_string(errno);
+                            const int err = 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()) {
-        std::string corrected_path = path;
-        if (endsWith(corrected_path, '/'))
-            corrected_path.erase(corrected_path.end() - 1);
+    if (path.empty())
+        return "no path specified";
 
-        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);
-}
+    std::string corrected_path = path;
+    if (endsWith(corrected_path, '/'))
+        corrected_path.erase(corrected_path.end() - 1);
 
-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);
+    return addFiles2(files, corrected_path, extra, recursive, ignored);
 }
 
 #endif

cli/filelister.h

@@ -73,18 +73,6 @@ class FileLister {
      * @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());
-    return f.is_open();
+    struct stat file_stat;
+    if (stat(path.c_str(), &file_stat) == -1)
+        return 0;
+    return file_stat.st_mode & S_IFMT;
 }
 
+bool Path::isFile(const std::string &path)
+{
+    return file_type(path) == S_IFREG;
+}
 
-bool Path::directoryExists(const std::string &path)
+bool Path::isDirectory(const std::string &path)
 {
-    struct stat info;
-    return stat(path.c_str(), &info) == 0 && (info.st_mode & S_IFDIR);
+    return file_type(path) == S_IFDIR;
 }
 
 std::string Path::join(std::string path1, std::string path2) {

lib/path.h

@@ -181,18 +181,18 @@ class CPPCHECKLIB Path {
     static std::string stripDirectoryPart(const std::string &file);
 
     /**
-     * @brief Checks if a File exists
-     * @param file Path to be checked if it is a File
-     * @return true if given path is a File
+     * @brief Checks if given path is a file
+     * @param path Path to be checked
+     * @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