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

[chaoren]

Summary

Fix FileSpec::Dump() to output denormalized path.

See D9942 for previous discussions.

[zturner]

looks good, just some style nitpicks.

include/lldb/Host/FileSpec.h

634 ↗

(On Diff #26646)

I think this should either be a const member function that operates on m_syntax or a file static function in the .cpp file.

source/Host/common/FileSpec.cpp

626 ↗

(On Diff #26646)

Why curly braces instead of parentheses?

829 ↗

(On Diff #26646)

I'm not sure what the style guide says (if anything) but I personally find it a little hard to read lots of "if (x) y;" on a single line. very close together.

[chaoren]

include/lldb/Host/FileSpec.h

634 ↗

(On Diff #26646)

Yeah, I agree it'd be better if it's contained in the .cpp file. Normalize/DeNormalize (and maybe even GetPathSeparator) should probably also be in there.

source/Host/common/FileSpec.cpp

626 ↗

(On Diff #26646)

Since we're using C++11, I prefer uniform initialization whenever possible.

829 ↗

(On Diff #26646)

Yeah, I have no strong opinion on this. Will update.

[chaoren]

• Address review comments.

[zturner]

Looks good.

lldb/trunk/include/lldb/Host/FileSpec.h

public:
     /// Dump this object to a Stream.
     ///
     /// Dump the object to the supplied stream \a s. If the object
     /// contains a valid directory name, it will be displayed followed
     /// by a directory delimiter, and the filename.
     ///
     /// @param[in] s
     ///     The stream to which to dump the object description.
-    ///
-    /// @param[in] trailing_slash
-    ///     If true and the file is a non root directory, then a trailing slash
-    ///     will be added.
     //------------------------------------------------------------------
     void
-    Dump(Stream *s, bool trailing_slash = true) const;
+    Dump(Stream *s) const;
 
     //------------------------------------------------------------------
     /// Existence test.
     ///
     /// @return
     ///     \b true if the file exists on disk, \b false otherwise.
     //------------------------------------------------------------------
     bool
public:
     /// @return
     ///     A shared pointer to the data. This shared pointer can
     ///     contain a NULL DataBuffer pointer, so the contained pointer
     ///     must be checked prior to using it.
     //------------------------------------------------------------------
     lldb::DataBufferSP
     ReadFileContentsAsCString(Error *error_ptr = NULL);
 
-    static void Normalize(llvm::SmallVectorImpl<char> &path, PathSyntax syntax = ePathSyntaxHostNative);
-    static void DeNormalize(llvm::SmallVectorImpl<char> &path, PathSyntax syntax = ePathSyntaxHostNative);
-
-
     //------------------------------------------------------------------
     /// Run through the input string, replaying the effect of any ".." and produce
     /// the resultant path.  The input path is not required to be in the host file system
     /// format, but it is required to be normalized to that system.
     ///
     /// @param[in] input
     ///     The input path to analyze.
     ///

lldb/trunk/source/Host/common/FileSpec.cpp

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-static bool
+namespace {
+
+bool
+PathSyntaxIsPosix(FileSpec::PathSyntax syntax)
+{
+    return (syntax == FileSpec::ePathSyntaxPosix ||
+            (syntax == FileSpec::ePathSyntaxHostNative &&
+             FileSystem::GetNativePathSyntax() == FileSpec::ePathSyntaxPosix));
+}
+
+char
+GetPathSeparator(FileSpec::PathSyntax syntax)
+{
+    return PathSyntaxIsPosix(syntax) ? '/' : '\\';
+}
+
+void
+Normalize(llvm::SmallVectorImpl<char> &path, FileSpec::PathSyntax syntax)
+{
+    if (PathSyntaxIsPosix(syntax)) return;
+
+    std::replace(path.begin(), path.end(), '\\', '/');
+    // Windows path can have \\ slashes which can be changed by replace
+    // call above to //. Here we remove the duplicate.
+    auto iter = std::unique ( path.begin(), path.end(),
+                               []( char &c1, char &c2 ){
+                                  return (c1 == '/' && c2 == '/');});
+    path.erase(iter, path.end());
+}
+
+void
+Denormalize(llvm::SmallVectorImpl<char> &path, FileSpec::PathSyntax syntax)
+{
+    if (PathSyntaxIsPosix(syntax)) return;
+
+    std::replace(path.begin(), path.end(), '/', '\\');
+}
+
+bool
 GetFileStats (const FileSpec *file_spec, struct stat *stats_ptr)
 {
     char resolved_path[PATH_MAX];
     if (file_spec->GetPath (resolved_path, sizeof(resolved_path)))
         return ::stat (resolved_path, stats_ptr) == 0;
     return false;
 }
 
+}
+
 // Resolves the username part of a path of the form ~user/other/directories, and
 // writes the result into dst_path.  This will also resolve "~" to the current user.
 // If you want to complete "~" to the list of users, pass it to ResolvePartialUsername.
 void
 FileSpec::ResolveUsername (llvm::SmallVectorImpl<char> &path)
 {
 #if LLDB_CONFIG_TILDE_RESOLVES_TO_USER
     if (path.empty() || path[0] != '~')
if (this != &rhs)
         m_directory = rhs.m_directory;
         m_filename = rhs.m_filename;
         m_is_resolved = rhs.m_is_resolved;
         m_syntax = rhs.m_syntax;
     }
     return *this;
 }
 
-void FileSpec::Normalize(llvm::SmallVectorImpl<char> &path, PathSyntax syntax)
-{
-    if (syntax == ePathSyntaxPosix ||
-        (syntax == ePathSyntaxHostNative && FileSystem::GetNativePathSyntax() == ePathSyntaxPosix))
-        return;
-
-    std::replace(path.begin(), path.end(), '\\', '/');
-    // Windows path can have \\ slashes which can be changed by replace
-    // call above to //. Here we remove the duplicate.
-    auto iter = std::unique ( path.begin(), path.end(),
-                               []( char &c1, char &c2 ){
-                                  return (c1 == '/' && c2 == '/');});
-    path.erase(iter, path.end());
-}
-
-void FileSpec::DeNormalize(llvm::SmallVectorImpl<char> &path, PathSyntax syntax)
-{
-    if (syntax == ePathSyntaxPosix ||
-        (syntax == ePathSyntaxHostNative && FileSystem::GetNativePathSyntax() == ePathSyntaxPosix))
-        return;
-
-    std::replace(path.begin(), path.end(), '/', '\\');
-}
-
 //------------------------------------------------------------------
 // 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
 // string values for quick comparison and efficient memory usage.
 //------------------------------------------------------------------
 void
 FileSpec::SetFile (const char *pathname, bool resolve, PathSyntax syntax)
 {

 }
 
 //------------------------------------------------------------------
 // Dump the object to the supplied stream. If the object contains
 // a valid directory name, it will be displayed followed by a
 // 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);
+        std::string path{GetPath(true)};
+        s->PutCString(path.c_str());
+        char path_separator = GetPathSeparator(m_syntax);
+        if (!m_filename && !path.empty() && path.back() != path_separator)
+            s->PutChar(path_separator);
     }
 }
 
 //------------------------------------------------------------------
 // Returns true if the file exists.
 //------------------------------------------------------------------
 bool
 FileSpec::Exists () const
FileSpec::GetPath(bool denormalize) const
     llvm::SmallString<64> result;
     GetPath(result, denormalize);
     return std::string(result.begin(), result.end());
 }
 
 void
 FileSpec::GetPath(llvm::SmallVectorImpl<char> &path, bool denormalize) const
 {
-    StreamString stream;
-    Dump(&stream, false);
-    path.append(stream.GetString().begin(), stream.GetString().end());
+    path.append(m_directory.GetStringRef().begin(), m_directory.GetStringRef().end());
+    if (m_directory)
+        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);
+        Denormalize(path, m_syntax);
 }
 
 ConstString
 FileSpec::GetFileNameExtension () const
 {
     if (m_filename)
     {
         const char *filename = m_filename.GetCString();

 bool
 FileSpec::IsRelativeToCurrentWorkingDirectory () const
 {
     const char *dir = m_directory.GetCString();
     llvm::StringRef directory(dir ? dir : "");
 
     if (directory.size() > 0)
     {
-        if (m_syntax == ePathSyntaxWindows)
-        {
-            if (directory.size() >= 2 && directory[1] == ':')
-                return false;
-            if (directory[0] == '/')
-                return false;
-            return true;
-        }
-        else
+        if (PathSyntaxIsPosix(m_syntax))
         {
             // If the path doesn't start with '/' or '~', return true
             switch (directory[0])
             {
                 case '/':
                 case '~':
                     return false;
                 default:
                     return true;
             }
         }
+        else
+        {
+            if (directory.size() >= 2 && directory[1] == ':')
+                return false;
+            if (directory[0] == '/')
+                return false;
+            return true;
+        }
     }
     else if (m_filename)
     {
         // No directory, just a basename, return true
         return true;
     }
     return false;
 }