improved `Path` handling of mixed separators (#4808)

lib/path.cpp

@@ -32,6 +32,7 @@
 #include <unistd.h>
 #else
 #include <direct.h>
+#include <stdexcept>
 #endif
 #if defined(__CYGWIN__)
 #include <strings.h>
@@ -150,9 +151,12 @@ bool Path::isAbsolute(const std::string& path)
     return false;
 }
 
-std::string Path::getRelativePath(const std::string& absolutePath, const std::vector<std::string>& basePaths)
+std::string Path::getRelativePath(std::string absolutePath, const std::vector<std::string>& basePaths)
 {
-    for (const std::string &bp : basePaths) {
+    absolutePath = Path::fromNativeSeparators(std::move(absolutePath));
+
+    for (std::string bp : basePaths) {
+        bp = Path::fromNativeSeparators(std::move(bp));
         if (absolutePath == bp || bp.empty()) // Seems to be a file, or path is empty
             continue;
 
@@ -161,7 +165,7 @@ std::string Path::getRelativePath(const std::string& absolutePath, const std::ve
 
         if (endsWith(bp,'/'))
             return absolutePath.substr(bp.length());
-        else if (absolutePath.size() > bp.size() && absolutePath[bp.length()] == '/')
+        if (absolutePath.size() > bp.size() && absolutePath[bp.length()] == '/')
             return absolutePath.substr(bp.length() + 1);
     }
     return absolutePath;
@@ -211,8 +215,12 @@ std::string Path::getAbsoluteFilePath(const std::string& filePath)
     if (_fullpath(absolute, filePath.c_str(), _MAX_PATH))
         absolute_path = absolute;
 #elif defined(__linux__) || defined(__sun) || defined(__hpux) || defined(__GNUC__) || defined(__CPPCHECK__)
+    // realpath() only works with files that actually exist
     char * absolute = realpath(filePath.c_str(), nullptr);
-    if (absolute)
+    if (!absolute) {
+        const int err = errno;
+        throw std::runtime_error("realpath() failed with " + std::to_string(err));
+    }
     absolute_path = absolute;
     free(absolute);
 #else
@@ -221,15 +229,11 @@ std::string Path::getAbsoluteFilePath(const std::string& filePath)
     return absolute_path;
 }
 
-std::string Path::stripDirectoryPart(const std::string &file)
+std::string Path::stripDirectoryPart(std::string file)
 {
-#if defined(_WIN32) && !defined(__MINGW32__)
+    file = fromNativeSeparators(std::move(file));
-    const char native = '\\';
-#else
-    const char native = '/';
-#endif
 
-    const std::string::size_type p = file.rfind(native);
+    const std::string::size_type p = file.rfind('/');
     if (p != std::string::npos) {
         return file.substr(p + 1);
     }
@@ -243,6 +247,8 @@ bool Path::fileExists(const std::string &file)
 }
 
 std::string Path::join(std::string path1, std::string path2) {
+    path1 = fromNativeSeparators(std::move(path1));
+    path2 = fromNativeSeparators(std::move(path2));
     if (path1.empty() || path2.empty())
         return path1 + path2;
     if (path2.front() == '/')

lib/path.h

@@ -117,7 +117,7 @@ public:
      * @param basePaths Paths to which it may be made relative.
      * @return relative path, if possible. Otherwise absolutePath is returned unchanged
      */
-    static std::string getRelativePath(const std::string& absolutePath, const std::vector<std::string>& basePaths);
+    static std::string getRelativePath(std::string absolutePath, const std::vector<std::string>& basePaths);
 
     /**
      * @brief Get an absolute file path from a relative one.
@@ -172,7 +172,7 @@ public:
      * @param file filename to be stripped. path info is optional
      * @return filename without directory path part.
      */
-    static std::string stripDirectoryPart(const std::string &file);
+    static std::string stripDirectoryPart(std::string file);
 
     /**
      * @brief Checks if a File exists

lib/platform.cpp

@@ -178,6 +178,8 @@ bool cppcheck::Platform::platform(const std::string& platformstr, std::string& e
         return false;
     }
     else {
+        if (verbose)
+            std::cout << "current working directory '" + Path::getCurrentPath() + "'" << std::endl;
         bool found = false;
         for (const std::string& path : paths) {
             if (verbose)
@@ -203,19 +205,19 @@ bool cppcheck::Platform::loadPlatformFile(const char exename[], const std::strin
     std::vector<std::string> filenames;
     filenames.push_back(filename);
     filenames.push_back(filename + ".xml");
-    filenames.push_back("platforms/" + filename);
+    filenames.push_back(Path::join("platforms", filename));
-    filenames.push_back("platforms/" + filename + ".xml");
+    filenames.push_back(Path::join("platforms", filename + ".xml"));
     if (exename && (std::string::npos != Path::fromNativeSeparators(exename).find('/'))) {
         filenames.push_back(Path::getPathFromFilename(Path::fromNativeSeparators(exename)) + filename);
-        filenames.push_back(Path::getPathFromFilename(Path::fromNativeSeparators(exename)) + "platforms/" + filename);
+        filenames.push_back(Path::getPathFromFilename(Path::fromNativeSeparators(exename)) + Path::join("platforms", filename));
-        filenames.push_back(Path::getPathFromFilename(Path::fromNativeSeparators(exename)) + "platforms/" + filename + ".xml");
+        filenames.push_back(Path::getPathFromFilename(Path::fromNativeSeparators(exename)) + Path::join("platforms", filename + ".xml"));
     }
 #ifdef FILESDIR
     std::string filesdir = FILESDIR;
     if (!filesdir.empty() && filesdir[filesdir.size()-1] != '/')
         filesdir += '/';
-    filenames.push_back(filesdir + ("platforms/" + filename));
+    filenames.push_back(filesdir + Path::join("platforms", filename));
-    filenames.push_back(filesdir + ("platforms/" + filename + ".xml"));
+    filenames.push_back(filesdir + Path::join("platforms", filename + ".xml"));
 #endif
 
     // open file..

lib/symboldatabase.cpp

@@ -7474,7 +7474,7 @@ void ValueType::setDebugPath(const Token* tok, SourceLocation ctx, SourceLocatio
     std::string file = ctx.file_name();
     if (file.empty())
         return;
-    std::string s = Path::stripDirectoryPart(file) + ":" + MathLib::toString(ctx.line()) + ": " + ctx.function_name() +
+    std::string s = Path::stripDirectoryPart(std::move(file)) + ":" + MathLib::toString(ctx.line()) + ": " + ctx.function_name() +
                     " => " + local.function_name();
     debugPath.emplace_back(tok, std::move(s));
 }

lib/valueflow.cpp

@@ -168,7 +168,7 @@ static void setSourceLocation(ValueFlow::Value& v,
     std::string file = ctx.file_name();
     if (file.empty())
         return;
-    std::string s = Path::stripDirectoryPart(file) + ":" + MathLib::toString(ctx.line()) + ": " + ctx.function_name() +
+    std::string s = Path::stripDirectoryPart(std::move(file)) + ":" + MathLib::toString(ctx.line()) + ": " + ctx.function_name() +
                     " => " + local.function_name() + ": " + debugString(v);
     v.debugPath.emplace_back(tok, std::move(s));
 }

test/testpath.cpp

@@ -38,6 +38,8 @@ private:
         TEST_CASE(is_cpp);
         TEST_CASE(get_path_from_filename);
         TEST_CASE(join);
+        TEST_CASE(getAbsoluteFilePath);
+        TEST_CASE(stripDirectoryPart);
     }
 
     void removeQuotationMarks() const {
@@ -93,6 +95,7 @@ private:
     }
 
     void getRelative() const {
+        {
             const std::vector<std::string> basePaths = {
                 "",     // Don't crash with empty paths
                 "C:/foo",
@@ -106,6 +109,35 @@ private:
             ASSERT_EQUALS("C:/test.cpp", Path::getRelativePath("C:/test.cpp", basePaths));
             ASSERT_EQUALS("C:/foobar/test.cpp", Path::getRelativePath("C:/foobar/test.cpp", basePaths));
         }
+        {
+            const std::vector<std::string> basePaths = {
+                "",     // Don't crash with empty paths
+                "C:\\foo",
+                "C:\\bar\\",
+                "C:\\test.cpp"
+            };
+
+            ASSERT_EQUALS("x.c", Path::getRelativePath("C:\\foo\\x.c", basePaths));
+            ASSERT_EQUALS("y.c", Path::getRelativePath("C:\\bar\\y.c", basePaths));
+            ASSERT_EQUALS("foo/y.c", Path::getRelativePath("C:\\bar\\foo\\y.c", basePaths));
+            ASSERT_EQUALS("C:/test.cpp", Path::getRelativePath("C:\\test.cpp", basePaths));
+            ASSERT_EQUALS("C:/foobar/test.cpp", Path::getRelativePath("C:\\foobar\\test.cpp", basePaths));
+        }
+        {
+            const std::vector<std::string> basePaths = {
+                "",     // Don't crash with empty paths
+                "/c/foo",
+                "/c/bar/",
+                "/c/test.cpp"
+            };
+
+            ASSERT_EQUALS("x.c", Path::getRelativePath("/c/foo/x.c", basePaths));
+            ASSERT_EQUALS("y.c", Path::getRelativePath("/c/bar/y.c", basePaths));
+            ASSERT_EQUALS("foo/y.c", Path::getRelativePath("/c/bar/foo\\y.c", basePaths));
+            ASSERT_EQUALS("/c/test.cpp", Path::getRelativePath("/c/test.cpp", basePaths));
+            ASSERT_EQUALS("/c/foobar/test.cpp", Path::getRelativePath("/c/foobar/test.cpp", basePaths));
+        }
+    }
 
     void is_c() const {
         ASSERT(Path::isC("index.cpp")==false);
@@ -141,6 +173,12 @@ private:
         ASSERT_EQUALS("/tmp/", Path::getPathFromFilename("/tmp/index.h"));
         ASSERT_EQUALS("a/b/c/", Path::getPathFromFilename("a/b/c/index.h"));
         ASSERT_EQUALS("a/b/c/", Path::getPathFromFilename("a/b/c/"));
+        ASSERT_EQUALS("S:\\tmp\\", Path::getPathFromFilename("S:\\tmp\\index.h"));
+        ASSERT_EQUALS("a\\b\\c\\", Path::getPathFromFilename("a\\b\\c\\index.h"));
+        ASSERT_EQUALS("a\\b\\c\\", Path::getPathFromFilename("a\\b\\c\\"));
+        ASSERT_EQUALS("S:\\a\\b\\c\\", Path::getPathFromFilename("S:\\a\\b\\c\\"));
+        ASSERT_EQUALS("S:/tmp/", Path::getPathFromFilename("S:/tmp/index.h"));
+        ASSERT_EQUALS("S:/a/b/c/", Path::getPathFromFilename("S:/a/b/c/index.h"));
     }
 
     void join() const {
@@ -148,7 +186,35 @@ private:
         ASSERT_EQUALS("a", Path::join("", "a"));
         ASSERT_EQUALS("a/b", Path::join("a", "b"));
         ASSERT_EQUALS("a/b", Path::join("a/", "b"));
+        ASSERT_EQUALS("a/b", Path::join("a\\", "b"));
         ASSERT_EQUALS("/b", Path::join("a", "/b"));
+        ASSERT_EQUALS("/b", Path::join("a", "\\b"));
+    }
+
+    // TODO: this is quite messy - should Path::getAbsoluteFilePath() return normalized separators?
+    void getAbsoluteFilePath() const {
+        // Path::getAbsoluteFilePath() only works with existing paths on Linux
+#ifdef _WIN32
+        const std::string cwd = Path::getCurrentPath();
+        ASSERT_EQUALS(Path::join(cwd, "a.h"), Path::fromNativeSeparators(Path::getAbsoluteFilePath("a.h")));
+        ASSERT_EQUALS(Path::join(cwd, "inc/a.h"), Path::fromNativeSeparators(Path::getAbsoluteFilePath("inc/a.h")));
+        const std::string cwd_down = Path::getPathFromFilename(cwd);
+        ASSERT_EQUALS(Path::join(cwd_down, "a.h"), Path::fromNativeSeparators(Path::getAbsoluteFilePath("../a.h")));
+        ASSERT_EQUALS(Path::join(cwd_down, "inc/a.h"), Path::fromNativeSeparators(Path::getAbsoluteFilePath("../inc/a.h")));
+        ASSERT_EQUALS(Path::join(cwd_down, "inc/a.h"), Path::fromNativeSeparators(Path::getAbsoluteFilePath("../inc/../inc/a.h")));
+#endif
+    }
+
+    void stripDirectoryPart() const {
+        ASSERT_EQUALS("a.h", Path::stripDirectoryPart("a.h"));
+        ASSERT_EQUALS("a.h", Path::stripDirectoryPart("a/a.h"));
+        ASSERT_EQUALS("a.h", Path::stripDirectoryPart("a/b/a.h"));
+        ASSERT_EQUALS("a.h", Path::stripDirectoryPart("/mnt/a/b/a.h"));
+        ASSERT_EQUALS("a.h", Path::stripDirectoryPart("a\\a.h"));
+        ASSERT_EQUALS("a.h", Path::stripDirectoryPart("a\\b\\a.h"));
+        ASSERT_EQUALS("a.h", Path::stripDirectoryPart("S:\\a\\b\\a.h"));
+        ASSERT_EQUALS("a.h", Path::stripDirectoryPart("S:/a/b/a.h"));
+
     }
 };