[LLVM] sys::path::replace_path_prefix fix and simplifications

[saudi]

Summary

When working on D76869, I discovered some failing scenarios for sys::path::replace_path_prefix.
However, this function is already a bit complex and the fix would make the logic harder to read.
Also, its parameters strict and style are always used with the default value.

This patch removes the 2 parameters, reducing the code greatly and made it easier to fix the failing cases.

Overall, I was planning to split my work in 3 steps:

1. This fix
2. finish D76869 (which this fix will make smaller/simpler)
3. Modifiy replace_prefix_map under windows, to replace startswith with a case and separator insensitive version (for example, C:\\OneFolder\\ would match c:/onefolder/file.cpp)

[MaskRay]

Thanks!

llvm/include/llvm/Support/Path.h

171

So, this change restores the original 3-parameter form of replace_path_prefix. LG

[saudi]

Added Style parameter to replace_path_prefix, as suggested.
This allowed to make the replace_path_prefix unit-test platform independent, removing a #ifdef _WIN32 that I added in a previous version of the patch.

The tests ninja check-all passed on windows.
@amccarth : Does it cover all the VFS tests you suggested that I double-check?

EDIT: Nevermind this update and comment, it was intended to another patch

llvm/include/llvm/Support/Path.h

 void replace_extension(SmallVectorImpl<char> &path, const Twine &extension,
                        Style style = Style::native);
 
 /// Replace matching path prefix with another path.
 ///
 /// @code
 ///   /foo, /old, /new => /foo
 ///   /old, /old, /new => /new
-///   /old, /old/, /new, false => /old
-///   /old, /old/, /new, true => /new
+///   /old, /old/, /new => /old
 ///   /old/foo, /old, /new => /new/foo
 ///   /old/foo, /old/, /new => /new/foo
 ///   /old/foo, /old/, /new/ => /new/foo
 ///   /oldfoo, /old, /new => /oldfoo
 ///   /foo, <empty>, /new => /new/foo
 ///   /foo, <empty>, new => new/foo
-///   /old/foo, /old, <empty>, false => /foo
-///   /old/foo, /old, <empty>, true => foo
+///   /old/foo, /old, <empty> => /foo
 /// @endcode
 ///
 /// @param Path If \a Path starts with \a OldPrefix modify to instead
 ///        start with \a NewPrefix.
-/// @param OldPrefix The path prefix to strip from \a Path. Any trailing
-///        path separator is ignored if strict is true.
+/// @param OldPrefix The path prefix to strip from \a Path.
 /// @param NewPrefix The path prefix to replace \a NewPrefix with.
-/// @param style The path separator style
-/// @param strict If strict is true, a directory separator following
-///        \a OldPrefix will also be stripped. Otherwise, directory
-///        separators will only be matched and stripped when present
-///        in \a OldPrefix.
 /// @result true if \a Path begins with OldPrefix
-bool replace_path_prefix(SmallVectorImpl<char> &Path,
-                         const StringRef &OldPrefix, const StringRef &NewPrefix,
-                         Style style = Style::native, bool strict = false);
+bool replace_path_prefix(SmallVectorImpl<char> &Path, StringRef OldPrefix,
+                         StringRef NewPrefix);
 
 /// Append to path.
 ///
 /// @code
 ///   /foo  + bar/f => /foo/bar/f
 ///   /foo/ + bar/f => /foo/bar/f
 ///   foo   + bar/f => foo/bar/f
 /// @endcode

llvm/lib/Support/Path.cpp

void replace_extension(SmallVectorImpl<char> &path, const Twine &extension,
   // Append '.' if needed.
   if (ext.size() > 0 && ext[0] != '.')
     path.push_back('.');
 
   // Append extension.
   path.append(ext.begin(), ext.end());
 }
 
-bool replace_path_prefix(SmallVectorImpl<char> &Path,
-                         const StringRef &OldPrefix, const StringRef &NewPrefix,
-                         Style style, bool strict) {
+bool replace_path_prefix(SmallVectorImpl<char> &Path, StringRef OldPrefix,
+                         StringRef NewPrefix) {
   if (OldPrefix.empty() && NewPrefix.empty())
     return false;
 
   StringRef OrigPath(Path.begin(), Path.size());
-  StringRef OldPrefixDir;
-
-  if (!strict && OldPrefix.size() > OrigPath.size())
-    return false;
-
-  // Ensure OldPrefixDir does not have a trailing separator.
-  if (!OldPrefix.empty() && is_separator(OldPrefix.back()))
-    OldPrefixDir = parent_path(OldPrefix, style);
-  else
-    OldPrefixDir = OldPrefix;
-
-  if (!OrigPath.startswith(OldPrefixDir))
-    return false;
-
-  if (OrigPath.size() > OldPrefixDir.size())
-    if (!is_separator(OrigPath[OldPrefixDir.size()], style) && strict)
-      return false;
+  if (!OrigPath.startswith(OldPrefix))
 
   // If prefixes have the same size we can simply copy the new one over.
-  if (OldPrefixDir.size() == NewPrefix.size() && !strict) {
+  if (OldPrefix.size() == NewPrefix.size()) {
     llvm::copy(NewPrefix, Path.begin());
     return true;
   }
 
-  StringRef RelPath = OrigPath.substr(OldPrefixDir.size());
+  StringRef RelPath = OrigPath.substr(OldPrefix.size());
   SmallString<256> NewPath;
-  path::append(NewPath, style, NewPrefix);
-  if (!RelPath.empty()) {
-    if (!is_separator(RelPath[0], style) || !strict)
-      path::append(NewPath, style, RelPath);
-    else
-      path::append(NewPath, style, relative_path(RelPath, style));
-  }
-
+  (Twine(NewPrefix) + RelPath).toVector(NewPath);
   Path.swap(NewPath);
-
   return true;
 }
 
 void native(const Twine &path, SmallVectorImpl<char> &result, Style style) {
   assert((!path.isSingleStringRef() ||
           path.getSingleStringRef().data() != result.data()) &&
          "path and result are not allowed to overlap!");
   // Clear result.

llvm/unittests/Support/Path.cpp

TEST(Support, ReplacePathPrefix) {
   EXPECT_EQ(Path, "/longernew/foo");
   Path = Path1;
   path::replace_path_prefix(Path, EmptyPrefix, NewPrefix);
   EXPECT_EQ(Path, "/new/foo");
   Path = Path2;
   path::replace_path_prefix(Path, OldPrefix, EmptyPrefix);
   EXPECT_EQ(Path, "/foo");
   Path = Path2;
-  path::replace_path_prefix(Path, OldPrefix, EmptyPrefix, path::Style::native,
-                            true);
+  path::replace_path_prefix(Path, OldPrefixSep, EmptyPrefix);
   EXPECT_EQ(Path, "foo");
   Path = Path3;
-  path::replace_path_prefix(Path, OldPrefix, NewPrefix, path::Style::native,
-                            false);
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
   EXPECT_EQ(Path, "/newnew/foo");
   Path = Path3;
-  path::replace_path_prefix(Path, OldPrefix, NewPrefix, path::Style::native,
-                            true);
-  EXPECT_EQ(Path, "/oldnew/foo");
-  Path = Path3;
-  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, path::Style::native,
-                            true);
-  EXPECT_EQ(Path, "/oldnew/foo");
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix2);
+  EXPECT_EQ(Path, "/longernewnew/foo");
   Path = Path1;
   path::replace_path_prefix(Path, EmptyPrefix, NewPrefix);
   EXPECT_EQ(Path, "/new/foo");
   Path = OldPrefix;
   path::replace_path_prefix(Path, OldPrefix, NewPrefix);
   EXPECT_EQ(Path, "/new");
   Path = OldPrefixSep;
   path::replace_path_prefix(Path, OldPrefix, NewPrefix);
   EXPECT_EQ(Path, "/new/");
   Path = OldPrefix;
-  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, path::Style::native,
-                            false);
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix);
   EXPECT_EQ(Path, "/old");
-  Path = OldPrefix;
-  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, path::Style::native,
-                            true);
-  EXPECT_EQ(Path, "/new");
 }
 
 TEST_F(FileSystemTest, OpenFileForRead) {
   // Create a temp file.
   int FileDescriptor;
   SmallString<64> TempPath;
   ASSERT_NO_ERROR(
       fs::createTemporaryFile("prefix", "temp", FileDescriptor, TempPath));