Make FileSpec::Dump use FileSpec::GetPath(), not the other way around.

[zturner]

Summary

We need FileSpec::Dump() to format the result according to the paths' internal path syntax. So this means backslashes on windows, for example. GetPath already has the ability to do this, but GetPath was calling Dump and then adding the denormalization logic on top of that. So the change here is to make it work the other way around. Have GetPath call dump instead, and then just append the trailing slash if necessary. This way Dump can just call GetPath(true).

[zturner]

source/Host/common/FileSpec.cpp

845

Ignore this line, I had it in for debugging purposes. Forgot to remove it before uploading.

[chaoren]

source/Host/common/FileSpec.cpp

628

How about just s->PutCString(GetPath().c_str())?

831

s/AsCString/GetStringRef/

839

See above.

[chaoren]

source/Host/common/FileSpec.cpp

619

Please ignore this comment. Phabricator won't let me delete.

629

We also need m_directory && !m_directory.GetStringRef().endswith("/")

[zturner]

source/Host/common/FileSpec.cpp

629

Ahh, I think I need to fix the endswith() check in GetPath instead. Previously GetPath() was calling Dump with trailing_slash=false, so that means anyone calling GetPath() directly was getting back something that never had a trailing slash. But with this change, it's possible for a direct call to GetPath() to include a trailing slash.

So if I fix it there, I can just remove this whole branch and the entire function is just

if (s)
    s->PutCString(GetPath().c_str());

[zturner]

Let's see if this is better. GetPath() should now NEVER append a trailing slash, and Dump() should now ALWAYS append a trailing slash. there were no users of Dump() passing false, so I assume the ability to choose wasn't needed, which is why I removed the argument.

[chaoren]

source/Host/common/FileSpec.cpp

630

What if path == "/"?

844

What if m_directory == '/'? We'd get //filename.

[chaoren]

source/Host/common/FileSpec.cpp

844

Nevermind, you trimmed away the single '/'. But there's another problem if m_directory == '/' and m_filename is empty, you return an empty string.

[zturner]

GetPath() is supposed to have not put a trailing slash in that case. See
the call to rtrim() in GetPath()

include/lldb/Host/FileSpec.h

     ///     will be added.
     //------------------------------------------------------------------
     void
-    Dump(Stream *s, bool trailing_slash = true) const;
+    Dump(Stream *s) const;
 
     //------------------------------------------------------------------
     /// Existence test.

     static void Normalize(llvm::SmallVectorImpl<char> &path, PathSyntax syntax = ePathSyntaxHostNative);
     static void DeNormalize(llvm::SmallVectorImpl<char> &path, PathSyntax syntax = ePathSyntaxHostNative);
 
+    char GetPathSeparator() const;
 
     //------------------------------------------------------------------
     /// Run through the input string, replaying the effect of any ".." and produce

source/Host/common/FileSpec.cpp

     std::replace(path.begin(), path.end(), '/', '\\');
 }
 
+char
+FileSpec::GetPathSeparator() const
+{
+    if (m_syntax == ePathSyntaxPosix ||
+        (m_syntax == ePathSyntaxHostNative && FileSystem::GetNativePathSyntax() == ePathSyntaxPosix))
+        return '/';
+
+    return '\\';
+}
+
 //------------------------------------------------------------------
 // Update the contents of this object with a new path. The path will
 // be split up into a directory and filename and stored as uniqued

 // directory delimiter, and the filename.
 //------------------------------------------------------------------
 void
-FileSpec::Dump(Stream *s, bool trailing_slash) const
+FileSpec::Dump(Stream *s) const
 {
     if (s)
     {
-        m_directory.Dump(s);
-        if ((m_filename || trailing_slash) && m_directory &&
-                !m_directory.GetStringRef().endswith("/"))
-            s->PutChar('/');
-        m_filename.Dump(s);
+        // Dump guarantees that directory-only paths will have a path separator
+        // appended.  GetPath() guarantees that directory-only paths will not have
+        // a path separator appended.  So it's safe to just append one.
+        std::string path = GetPath();
+        if (!m_filename && !path.empty())
+            path.push_back(GetPathSeparator());
+        s->PutCString(path.c_str());
     }
 }
 

 void
 FileSpec::GetPath(llvm::SmallVectorImpl<char> &path, bool denormalize) const
 {
-    StreamString stream;
-    Dump(&stream, false);
-    path.append(stream.GetString().begin(), stream.GetString().end());
-    Normalize(path, m_syntax);
+    if (m_directory)
+    {
+        llvm::StringRef dir = m_directory.GetStringRef();
+        // Don't add any trailing slashes, even if `m_directory` has one.  This
+        // will guarantee that GetPath() returns paths with no trailing slash if
+        // there is no filename.  If there is a filename, we will join the directory
+        // and file with a slash later.
+        dir = dir.rtrim("/\\");
+        path.append(dir.begin(), dir.end());
+    }
+
+    if (m_filename)
+    {
+        if (m_directory)
+            path.push_back('/');
+        llvm::StringRef file = m_filename.GetStringRef();
+        path.append(file.begin(), file.end());
+    }
+
     if (denormalize && !path.empty())
         DeNormalize(path, m_syntax);
 }