Fix llvm::sys::path::remove_dots() to return "." instead of an empty path.

[clayborg]

Summary

LLDB recently started using llvm::sys::path::remove_dots() to normalize paths inside of LLDB. We ran into a few cases where this was falling down for us where paths that resulted in the current working directory (".", "./", "foo/.." and more) would result in an empty file specification since llvm::sys::path::remove_dots() would return an empty path. This fix corrects this issue by having "." returned if the path specified was not empty to begin with and the resulting path would have been empty. Pavel and I believe this is the intention of this function. If this is not correct please comment.

[clayborg]

Discussion of the reasoning for this is available in the LLDB patch: https://reviews.llvm.org/D46783

[labath]

As I said on the lldb patch, I believe this behavior makes more sense for a function like this (i.e., it should never return an invalid path ("") if the input path is valid, even if it means leaving some dots around. (We already keep a leading ../ component for the remove_dot_dot=true case as it is not possible to preserve the path semantics without this.)

unittests/Support/Path.cpp

1166 ↗

(On Diff #146851)

Another interesting test here would be to make sure this does return an empty path when the input is also empty.

[clayborg]

unittests/Support/Path.cpp

1166 ↗

(On Diff #146851)

I will add one.

[clayborg]

Added a test that tests for an empty path and making sure remove_dots leaves it empty.

[ruiu]

I believe this function was written based on Rob Pike's Clean() function for Go, and the Go's function also returns "." (instead of "") for the current directory. So this change makes sense to me.

https://golang.org/pkg/path/#example_Clean

Maybe you should return "." for "". as well. An empty string as a path component makes sense, as "/foo/bar", "/foo//bar" and "/foo/./bar" are all interpreted as the same path. Go's Clean() returns "." for "" too.

[labath]

In D46887#1100111, @ruiu wrote:

I believe this function was written based on Rob Pike's Clean() function for Go, and the Go's function also returns "." (instead of "") for the current directory. So this change makes sense to me.

https://golang.org/pkg/path/#example_Clean

Maybe you should return "." for "". as well. An empty string as a path component makes sense, as "/foo/bar", "/foo//bar" and "/foo/./bar" are all interpreted as the same path. Go's Clean() returns "." for "" too.

I was surprised by this behavior, so I checked what python does in this case. And indeed, os.path.normpath("") will also return ".". So, it seems there is an established behavior pattern for this input among functions of this type, and it would be best to follow that (if that doesn't cause too much havoc among existing users).

[clayborg]

Fix empty path to become "." to match python and Go's path utilities.

[ruiu]

LGTM

[clayborg]

Reverted 332508 as it was causing clang test suite problems.

[clayborg]

So many places that are manually calling remove_dots are expecting an empty path in clang. I am also not sure if any other clients, like lld or other source bases won't be affected so I am going to abandon this patch

llvm/trunk/lib/Support/Path.cpp

if (remove_dot_dot && C == "..") {
         continue;
     }
     components.push_back(C);
   }
 
   SmallString<256> buffer = path::root_path(path, style);
   for (StringRef C : components)
     path::append(buffer, style, C);
+  // If the buffer is empty, then return ".". Many other path utilities
+  // do this so it seems to be an expected result.
+  if (buffer.empty())
+    buffer.append(1, '.');
   return buffer;
 }
 
 bool remove_dots(SmallVectorImpl<char> &path, bool remove_dot_dot,
                  Style style) {
   StringRef p(path.data(), path.size());
 
   SmallString<256> result = remove_dots(p, remove_dot_dot, style);

llvm/trunk/unittests/Support/Path.cpp

static std::string remove_dots(StringRef path, bool remove_dot_dot,
   SmallString<256> buffer(path);
   path::remove_dots(buffer, remove_dot_dot, style);
   return buffer.str();
 }
 
 TEST(Support, RemoveDots) {
   EXPECT_EQ("foolz\\wat",
             remove_dots(".\\.\\\\foolz\\wat", false, path::Style::windows));
-  EXPECT_EQ("", remove_dots(".\\\\\\\\\\", false, path::Style::windows));
+  EXPECT_EQ(".", remove_dots(".\\\\\\\\\\", false, path::Style::windows));
 
   EXPECT_EQ("a\\..\\b\\c",
             remove_dots(".\\a\\..\\b\\c", false, path::Style::windows));
   EXPECT_EQ("b\\c", remove_dots(".\\a\\..\\b\\c", true, path::Style::windows));
   EXPECT_EQ("c", remove_dots(".\\.\\c", true, path::Style::windows));
   EXPECT_EQ("..\\a\\c",
             remove_dots("..\\a\\b\\..\\c", true, path::Style::windows));
   EXPECT_EQ("..\\..\\a\\c",
             remove_dots("..\\..\\a\\b\\..\\c", true, path::Style::windows));
 
   SmallString<64> Path1(".\\.\\c");
   EXPECT_TRUE(path::remove_dots(Path1, true, path::Style::windows));
   EXPECT_EQ("c", Path1);
 
   EXPECT_EQ("foolz/wat",
             remove_dots("././/foolz/wat", false, path::Style::posix));
-  EXPECT_EQ("", remove_dots("./////", false, path::Style::posix));
+  EXPECT_EQ(".", remove_dots("./////", false, path::Style::posix));
+  EXPECT_EQ(".", remove_dots("", false, path::Style::posix));
 
   EXPECT_EQ("a/../b/c", remove_dots("./a/../b/c", false, path::Style::posix));
   EXPECT_EQ("b/c", remove_dots("./a/../b/c", true, path::Style::posix));
   EXPECT_EQ("c", remove_dots("././c", true, path::Style::posix));
   EXPECT_EQ("../a/c", remove_dots("../a/b/../c", true, path::Style::posix));
   EXPECT_EQ("../../a/c",
             remove_dots("../../a/b/../c", true, path::Style::posix));
   EXPECT_EQ("/a/c", remove_dots("/../../a/c", true, path::Style::posix));