Fix a couple of cornercases in FileSpec + tests

[labath]

Summary

This fixes a couple of corner cases in FileSpec, related to AppendPathComponent and
handling of root directory (/) file spec. I add a bunch of unit tests for the new behavior.

Summary of changes:

FileSpec("/bar").GetCString(): before "//bar", after "/bar".
FileSpec("/").CopyByAppendingPathComponent("bar").GetCString(): before "//bar", after "/bar".
FileSpec("C:", ePathSyntaxWindows).CopyByAppendingPathComponent("bar").GetCString(): before "C:/bar", after "C:\bar".

[labath]

unittests/Host/FileSpecTest.cpp

57

I've also identified one more issue in windows path handling (GetDirectory() always returns posix-style paths), which I do not attempt to address right now.

[clayborg]

Looks good.

[zturner]

This actually broke a couple of unit tests on Windows. Did you try running
these tests on Windows?

[labath]

Interesting... I didn't run the tests on windows. I was hoping they would run mostly the same on all platforms. I'll try to give them a spin now...

[labath]

lldb/trunk/unittests/Host/FileSpecTest.cpp

25 ↗

(On Diff #50396)

So this returns F: on linux and F:\ on Windows. This happens because llvm::sys::path::parent_path does not recognize F: as a "root directory" on linux, and therefore treats it differently. I don't know which behavior is more "correct" (probably the windows one), but I think that this should be consistent, regardless of the platform the test is run on (my original motivation for writing this was the fact that i was getting wonky paths while attempting to write other unit tests). Unfortunately, I think that means getting rid of llvm's path processing library...

What do you make of that?

(I am going on holiday, so I cannot to anything about this now. if you want to have a clean test run in the mean time, I am fine commenting these checks out or something...)

[zturner]

Yea, because of the fact that we must support both syntaxes on both
platforms, LLVM library is out.

The whole motivation for introducing the path syntax is so that windows
paths behave as if on Windows even if on linux

source/Host/common/FileSpec.cpp

 {
     return ConstString{GetPath(denormalize)}.AsCString(NULL);
 }
 
 void
 FileSpec::GetPath(llvm::SmallVectorImpl<char> &path, bool denormalize) const
 {
     path.append(m_directory.GetStringRef().begin(), m_directory.GetStringRef().end());
-    if (m_directory)
+    if (m_directory && !(m_directory.GetLength() == 1 && m_directory.GetCString()[0] == '/'))
         path.insert(path.end(), '/');
     path.append(m_filename.GetStringRef().begin(), m_filename.GetStringRef().end());
     Normalize(path, m_syntax);
     if (path.size() > 1 && path.back() == '/') path.pop_back();
     if (denormalize && !path.empty())
         Denormalize(path, m_syntax);
 }
 
return ForEachItemInDirectory(dir_path,
                 }
                 return eEnumerateDirectoryResultNext;
             });
 }
 
 FileSpec
 FileSpec::CopyByAppendingPathComponent (const char *new_path)  const
 {
-    const bool resolve = false;
-    if (m_filename.IsEmpty() && m_directory.IsEmpty())
-        return FileSpec(new_path,resolve);
-    StreamString stream;
-    if (m_filename.IsEmpty())
-        stream.Printf("%s/%s",m_directory.GetCString(),new_path);
-    else if (m_directory.IsEmpty())
-        stream.Printf("%s/%s",m_filename.GetCString(),new_path);
-    else
-        stream.Printf("%s/%s/%s",m_directory.GetCString(), m_filename.GetCString(),new_path);
-    return FileSpec(stream.GetData(),resolve);
+    FileSpec ret = *this;
+    ret.AppendPathComponent(new_path);
+    return ret;
 }
 
 FileSpec
 FileSpec::CopyByRemovingLastPathComponent ()  const
 {
     const bool resolve = false;
     if (m_filename.IsEmpty() && m_directory.IsEmpty())
         return FileSpec("",resolve);

 {
     return PrependPathComponent(new_path.GetPath(false));
 }
 
 void
 FileSpec::AppendPathComponent(const char *new_path)
 {
     if (!new_path) return;
-    const bool resolve = false;
-    if (m_filename.IsEmpty() && m_directory.IsEmpty())
+
+    StreamString stream;
+    if (!m_directory.IsEmpty())
     {
-        SetFile(new_path, resolve);
-        return;
+        stream.PutCString(m_directory.GetCString());
+        if (m_directory.GetLength() != 1 || m_directory.GetCString()[0] != '/')
+            stream.PutChar('/');
     }
-    StreamString stream;
-    if (m_filename.IsEmpty() || (m_filename.GetLength() == 1 && m_filename.GetCString()[0] == '.'))
-        stream.Printf("%s/%s", m_directory.GetCString(), new_path);
-    else if (m_directory.IsEmpty())
-        stream.Printf("%s/%s", m_filename.GetCString(), new_path);
-    else
-        stream.Printf("%s/%s/%s", m_directory.GetCString(), m_filename.GetCString(), new_path);
-    SetFile(stream.GetData(), resolve);
+
+    if (!m_filename.IsEmpty())
+    {
+        stream.PutCString(m_filename.GetCString());
+        if (m_filename.GetLength() != 1 || m_filename.GetCString()[0] != '/')
+            stream.PutChar('/');
+    }
+
+    stream.PutCString(new_path);
+
+    const bool resolve = false;
+    SetFile(stream.GetData(), resolve, m_syntax);
 }
 
 void
 FileSpec::AppendPathComponent(const std::string &new_path)
 {
     return AppendPathComponent(new_path.c_str());
 }
 

unittests/Host/CMakeLists.txt

 add_lldb_unittest(HostTests
+  FileSpecTest.cpp
   SocketAddressTest.cpp
   SocketTest.cpp
   SymbolsTest.cpp
   )

unittests/Host/FileSpecTest.cpp

+//===-- FileSpecTest.cpp ----------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Host/FileSpec.h"
+
+using namespace lldb_private;
+
+TEST(FileSpecTest, FileAndDirectoryComponents)
+{
+    FileSpec fs_posix("/foo/bar", false, FileSpec::ePathSyntaxPosix);
+    EXPECT_STREQ("/foo/bar", fs_posix.GetCString());
+    EXPECT_STREQ("/foo", fs_posix.GetDirectory().GetCString());
+    EXPECT_STREQ("bar", fs_posix.GetFilename().GetCString());
+
+    FileSpec fs_windows("F:\\bar", false, FileSpec::ePathSyntaxWindows);
+    EXPECT_STREQ("F:\\bar", fs_windows.GetCString());
+    EXPECT_STREQ("F:", fs_windows.GetDirectory().GetCString());
+    EXPECT_STREQ("bar", fs_windows.GetFilename().GetCString());
+
+    FileSpec fs_posix_root("/", false, FileSpec::ePathSyntaxPosix);
+    EXPECT_STREQ("/", fs_posix_root.GetCString());
+    EXPECT_EQ(nullptr, fs_posix_root.GetDirectory().GetCString());
+    EXPECT_STREQ("/", fs_posix_root.GetFilename().GetCString());
+
+    FileSpec fs_windows_root("F:", false, FileSpec::ePathSyntaxWindows);
+    EXPECT_STREQ("F:", fs_windows_root.GetCString());
+    EXPECT_EQ(nullptr, fs_windows_root.GetDirectory().GetCString());
+    EXPECT_STREQ("F:", fs_windows_root.GetFilename().GetCString());
+
+    FileSpec fs_posix_long("/foo/bar/baz", false, FileSpec::ePathSyntaxPosix);
+    EXPECT_STREQ("/foo/bar/baz", fs_posix_long.GetCString());
+    EXPECT_STREQ("/foo/bar", fs_posix_long.GetDirectory().GetCString());
+    EXPECT_STREQ("baz", fs_posix_long.GetFilename().GetCString());
+
+    FileSpec fs_windows_long("F:\\bar\\baz", false, FileSpec::ePathSyntaxWindows);
+    EXPECT_STREQ("F:\\bar\\baz", fs_windows_long.GetCString());
+    // We get "F:/bar" instead.
+    // EXPECT_STREQ("F:\\bar", fs_windows_long.GetDirectory().GetCString());
+    EXPECT_STREQ("baz", fs_windows_long.GetFilename().GetCString());
+
+    FileSpec fs_posix_trailing_slash("/foo/bar/", false, FileSpec::ePathSyntaxPosix);
+    EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString());
+    EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString());
+    EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString());
+
+    FileSpec fs_windows_trailing_slash("F:\\bar\\", false, FileSpec::ePathSyntaxWindows);
+    EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString());
+    // We get "F:/bar" instead.
+    // EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetDirectory().GetCString());
+    EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString());
+}
+
+TEST(FileSpecTest, AppendPathComponent)
+{
+    FileSpec fs_posix("/foo", false, FileSpec::ePathSyntaxPosix);
+    fs_posix.AppendPathComponent("bar");
+    EXPECT_STREQ("/foo/bar", fs_posix.GetCString());
+    EXPECT_STREQ("/foo", fs_posix.GetDirectory().GetCString());
+    EXPECT_STREQ("bar", fs_posix.GetFilename().GetCString());
+
+    FileSpec fs_windows("F:", false, FileSpec::ePathSyntaxWindows);
+    fs_windows.AppendPathComponent("bar");
+    EXPECT_STREQ("F:\\bar", fs_windows.GetCString());
+    EXPECT_STREQ("F:", fs_windows.GetDirectory().GetCString());
+    EXPECT_STREQ("bar", fs_windows.GetFilename().GetCString());
+
+    FileSpec fs_posix_root("/", false, FileSpec::ePathSyntaxPosix);
+    fs_posix_root.AppendPathComponent("bar");
+    EXPECT_STREQ("/bar", fs_posix_root.GetCString());
+    EXPECT_STREQ("/", fs_posix_root.GetDirectory().GetCString());
+    EXPECT_STREQ("bar", fs_posix_root.GetFilename().GetCString());
+
+    FileSpec fs_windows_root("F:", false, FileSpec::ePathSyntaxWindows);
+    fs_windows_root.AppendPathComponent("bar");
+    EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString());
+    EXPECT_STREQ("F:", fs_windows_root.GetDirectory().GetCString());
+    EXPECT_STREQ("bar", fs_windows_root.GetFilename().GetCString());
+}
+
+TEST(FileSpecTest, CopyByAppendingPathComponent)
+{
+    FileSpec fs = FileSpec("/foo", false, FileSpec::ePathSyntaxPosix).CopyByAppendingPathComponent("bar");
+    EXPECT_STREQ("/foo/bar", fs.GetCString());
+    EXPECT_STREQ("/foo", fs.GetDirectory().GetCString());
+    EXPECT_STREQ("bar", fs.GetFilename().GetCString());
+}