[FileSpec] Delegate common operations to llvm::sys::path

[JDevlieghere]

Summary

With the recent changes in FileSpec to use LLVM's path style, it is
possible to delegate a bunch of common path operations to LLVM's path
implementation. This means we only have to maintain a single
implementation and can benefit from the efforts of the rest of the LLVM
community.

This is part one of a set of patches. There was no obvious way to split
this so I just worked from top to bottom.

[zturner]

Looks like good cleanup to me, thanks!

source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

2593–2596 ↗

(On Diff #150977)

Can you change these strcmp operations to StringRef calls? Might as well do some cleanup if we're here anyway.

[labath]

source/Utility/FileSpec.cpp

244–246 ↗

(On Diff #150977)

I don't really have a clear opinion on whether the new behaviour here is better or not, but it any case this sounds like something worth explicitly calling out.

unittests/Utility/FileSpecTest.cpp

284 ↗

(On Diff #150977)

Any particular reason for removing this?

[JDevlieghere]

source/Utility/FileSpec.cpp

244–246 ↗

(On Diff #150977)

As far as the FileSpec class is concerned this doesn't change anything, we were always explicitly passing the m_style argument which is now implicit. To me this seems strictly better than what is was, were you might accidentally end up with the host path.

I'll add a comment in the header.

unittests/Utility/FileSpecTest.cpp

284 ↗

(On Diff #150977)

Yup, llvm doesn't consider C: or //net to be absolute paths (as opposed to C:\ or //net/. While this is debatable, it makes things consistent with drive names on Windows, which is why I added the net tests above.

[labath]

source/Utility/FileSpec.cpp

244–246 ↗

(On Diff #150977)

Yes, but the counterargument to that is: since you're replacing the old filespec with a completely unrelated path, who's to say that the old path syntax is any better fit than "host".

I think it would be best here to actually make this argument mandatory and force everyone to think about the correct syntax when constructing the object, but maybe that's a change for another day.

unittests/Utility/FileSpecTest.cpp

284 ↗

(On Diff #150977)

Ah, I see.. I can certainly understand why c: wouldn't be considered absolute, but I'm not sure the same thing applies to //net. However, I think I can live that interpretation right now..

[JDevlieghere]

source/Utility/FileSpec.cpp

244–246 ↗

(On Diff #150977)

Alright, I see your point, I was only thinking about its uses within the FileSpec class. I think we can solve both issues by making it mandatory publicly, and have a private function that maintains the current style.

[davide]

LGTM with a nit.

source/Utility/FileSpec.cpp

18–21 ↗

(On Diff #150977)

This comments are really too trivial to be useful. Can you remove them?

[labath]

source/Utility/FileSpec.cpp

244–246 ↗

(On Diff #150977)

Agreed, maintaining the style for internal uses of SetFile makes perfect sense.

[clayborg]

source/Utility/FileSpec.cpp

593 ↗

(On Diff #150977)

We won't need the full path in order to get the extension. Should we just get the m_filename as a string here?

599 ↗

(On Diff #150977)

We won't need the full path in order to get the extension. Should we just get the m_filename as a string here?

[JDevlieghere]

source/Utility/FileSpec.cpp

244–246 ↗

(On Diff #150977)

This is pretty intrusive so I'll do this in a follow-up commit.

lldb/trunk/include/lldb/Utility/FileSpec.h

 // C++ Includes
 #include <functional>
 #include <string>
 
 // Other libraries and framework includes
 // Project includes
 #include "lldb/Utility/ConstString.h"
 
-#include "llvm/ADT/StringRef.h" // for StringRef
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 
 #include <stddef.h> // for size_t
 #include <stdint.h> // for uint32_t, uint64_t
 
 namespace lldb_private {

 ///   =======================================================
 ///
 /// Any other value is considered an invalid format string.
 ///
 template <> struct format_provider<lldb_private::FileSpec> {
   static void format(const lldb_private::FileSpec &F, llvm::raw_ostream &Stream,
                      StringRef Style);
 };
-}
+} // namespace llvm
 
 #endif // liblldb_FileSpec_h_

lldb/trunk/source/Core/Debugger.cpp

bool Debugger::LoadPlugin(const FileSpec &spec, Status &error) {
   return false;
 }
 
 static FileSpec::EnumerateDirectoryResult
 LoadPluginCallback(void *baton, llvm::sys::fs::file_type ft,
                    const FileSpec &file_spec) {
   Status error;
 
-  static ConstString g_dylibext("dylib");
-  static ConstString g_solibext("so");
+  static ConstString g_dylibext(".dylib");
+  static ConstString g_solibext(".so");
 
   if (!baton)
     return FileSpec::eEnumerateDirectoryResultQuit;
 
   Debugger *debugger = (Debugger *)baton;
 
   namespace fs = llvm::sys::fs;
   // If we have a regular file, a symbolic link or unknown file type, try and

lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

unsigned ObjectFileELF::ParseSymbols(Symtab *symtab, user_id_t start_id,
   // instruction emulation based unwind plans try to emulate all instructions
   // in these symbols. Don't add these symbols to the symbol list as they have
   // no use for the debugger and they are causing a lot of trouble. Filtering
   // can't be restricted to Android because this special object file don't
   // contain the note section specifying the environment to Android but the
   // custom extension and file name makes it highly unlikely that this will
   // collide with anything else.
   ConstString file_extension = m_file.GetFileNameExtension();
-  bool skip_oatdata_oatexec = file_extension == ConstString("oat") ||
-                              file_extension == ConstString("odex");
+  bool skip_oatdata_oatexec = file_extension == ConstString(".oat") ||
+                              file_extension == ConstString(".odex");
 
   ArchSpec arch;
   GetArchitecture(arch);
   ModuleSP module_sp(GetModule());
   SectionList *module_section_list =
       module_sp ? module_sp->GetSectionList() : nullptr;
 
   // Local cache to avoid doing a FindSectionByName for each symbol. The "const

lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp

uint32_t PlatformAndroid::GetSdkVersion() {
   m_sdk_version = StringConvert::ToUInt32(version_string.c_str());
   return m_sdk_version;
 }
 
 Status PlatformAndroid::DownloadSymbolFile(const lldb::ModuleSP &module_sp,
                                            const FileSpec &dst_file_spec) {
   // For oat file we can try to fetch additional debug info from the device
   ConstString extension = module_sp->GetFileSpec().GetFileNameExtension();
-  if (extension != ConstString("oat") && extension != ConstString("odex"))
+  if (extension != ConstString(".oat") && extension != ConstString(".odex"))
     return Status(
         "Symbol file downloading only supported for oat and odex files");
 
   // If we have no information about the platform file we can't execute oatdump
   if (!module_sp->GetPlatformFileSpec())
     return Status("No platform file specified");
 
   // Symbolizer isn't available before SDK version 23

lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp

FileSpec::EnumerateDirectory(dir.c_str(), find_directories, find_files,
                                find_other, FindKDKandSDKDirectoriesInDirectory,
                                this);
 }
 
 // Helper function to find *.sdk and *.kdk directories in a given directory.
 FileSpec::EnumerateDirectoryResult
 PlatformDarwinKernel::FindKDKandSDKDirectoriesInDirectory(
     void *baton, llvm::sys::fs::file_type ft, const FileSpec &file_spec) {
-  static ConstString g_sdk_suffix = ConstString("sdk");
-  static ConstString g_kdk_suffix = ConstString("kdk");
+  static ConstString g_sdk_suffix = ConstString(".sdk");
+  static ConstString g_kdk_suffix = ConstString(".kdk");
 
   PlatformDarwinKernel *thisp = (PlatformDarwinKernel *)baton;
   if (ft == llvm::sys::fs::file_type::directory_file &&
       (file_spec.GetFileNameExtension() == g_sdk_suffix ||
        file_spec.GetFileNameExtension() == g_kdk_suffix)) {
     AddRootSubdirsToSearchPaths(thisp, file_spec.GetPath());
   }
   return FileSpec::eEnumerateDirectoryResultNext;
PlatformDarwinKernel::GetKernelsAndKextsInDirectoryNoRecursion(
     void *baton, llvm::sys::fs::file_type ft, const FileSpec &file_spec) {
   return GetKernelsAndKextsInDirectoryHelper(baton, ft, file_spec, false);
 }
 
 FileSpec::EnumerateDirectoryResult
 PlatformDarwinKernel::GetKernelsAndKextsInDirectoryHelper(
     void *baton, llvm::sys::fs::file_type ft, const FileSpec &file_spec,
     bool recurse) {
-  static ConstString g_kext_suffix = ConstString("kext");
-  static ConstString g_dsym_suffix = ConstString("dSYM");
+  static ConstString g_kext_suffix = ConstString(".kext");
+  static ConstString g_dsym_suffix = ConstString(".dSYM");
   static ConstString g_bundle_suffix = ConstString("Bundle");
   ConstString file_spec_extension = file_spec.GetFileNameExtension();
 
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
   Log *log_verbose(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM | LLDB_LOG_OPTION_VERBOSE));
 
   if (log_verbose)
       log_verbose->Printf ("PlatformDarwinKernel examining '%s'", file_spec.GetPath().c_str());

lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

if (ec || st.type() == fs::file_type::status_error ||
       if (!syspath_retval) {
         error.SetErrorString("Python sys.path handling failed");
         return false;
       }
 
       // strip .py or .pyc extension
       ConstString extension = target_file.GetFileNameExtension();
       if (extension) {
-        if (::strcmp(extension.GetCString(), "py") == 0)
+        if (llvm::StringRef(extension.GetCString()) == ".py")
           basename.resize(basename.length() - 3);
-        else if (::strcmp(extension.GetCString(), "pyc") == 0)
+        else if (llvm::StringRef(extension.GetCString()) == ".pyc")
           basename.resize(basename.length() - 4);
       }
     } else {
       error.SetErrorString("no known way to import this module specification");
       return false;
     }
 
     // check if the module is already import-ed

lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

if (die && die.HasChildren() == false) {
             // a *.dwo file which has DW_AT_GNU_dwo_name, but no
             // DW_AT_comp_dir. In this case the method
             // ModuleList::GetSharedModule will fail and the warning will be
             // printed. However, as one can notice in this case we don't
             // actually need to try to load the already loaded module
             // (corresponding to .dwo) so we simply skip it.
             if (m_obj_file->GetFileSpec()
                         .GetFileNameExtension()
-                        .GetStringRef() == "dwo" &&
+                        .GetStringRef() == ".dwo" &&
                 llvm::StringRef(m_obj_file->GetFileSpec().GetPath())
                     .endswith(dwo_module_spec.GetFileSpec().GetPath())) {
               continue;
             }
 
             Status error = ModuleList::GetSharedModule(
                 dwo_module_spec, module_sp, NULL, NULL, NULL);
             if (!module_sp) {

lldb/trunk/source/Utility/FileSpec.cpp

 //===-- FileSpec.cpp --------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/TildeExpressionResolver.h"
 
-#include "llvm/ADT/SmallString.h" // for SmallString
-#include "llvm/ADT/SmallVector.h" // for SmallVectorTemplat...
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/Triple.h"         // for Triple
-#include "llvm/ADT/Twine.h"          // for Twine
-#include "llvm/Support/ErrorOr.h"    // for ErrorOr
+#include "llvm/ADT/Triple.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h" // for raw_ostream, fs
+#include "llvm/Support/raw_ostream.h"
 
 #include <algorithm>    // for replace, min, unique
 #include <system_error> // for error_code
 #include <vector>       // for vector
 
 #include <assert.h> // for assert
 #include <stdio.h>  // for size_t, NULL, snpr...
 #include <string.h> // for strcmp

 
 bool PathStyleIsPosix(FileSpec::Style style) {
   return (style == FileSpec::Style::posix ||
           (style == FileSpec::Style::native &&
            GetNativeStyle() == FileSpec::Style::posix));
 }
 
 const char *GetPathSeparators(FileSpec::Style style) {
-  return PathStyleIsPosix(style) ? "/" : "\\/";
+  return llvm::sys::path::get_separator(style).data();
 }
 
 char GetPreferredPathSeparator(FileSpec::Style style) {
   return GetPathSeparators(style)[0];
 }
 
 bool IsPathSeparator(char value, FileSpec::Style style) {
   return value == '/' || (!PathStyleIsPosix(style) && value == '\\');
 }
 
 void Denormalize(llvm::SmallVectorImpl<char> &path, FileSpec::Style style) {
   if (PathStyleIsPosix(style))
     return;
 
   std::replace(path.begin(), path.end(), '/', '\\');
 }
-  
-bool PathIsRelative(llvm::StringRef path, FileSpec::Style style) {
-  
-  if (path.empty())
-    return false;
-
-  if (PathStyleIsPosix(style)) {
-    // If the path doesn't start with '/' or '~', return true
-    switch (path[0]) {
-      case '/':
-      case '~':
-        return false;
-      default:
-        return true;
-    }
-  } else {
-    if (path.size() >= 2 && path[1] == ':')
-      return false;
-    if (path[0] == '/')
-      return false;
-    return true;
-  }
-  return false;
-}
-
 } // end anonymous namespace
 
 void FileSpec::Resolve(llvm::SmallVectorImpl<char> &path) {
   if (path.empty())
     return;
 
   llvm::SmallString<32> Source(path.begin(), path.end());
   StandardTildeExpressionResolver Resolver;
switch (next) {
 
       default:
         break;
     }
   }
   return false;
 }
 
-  
 }
 //------------------------------------------------------------------
 // Assignment operator.
 //------------------------------------------------------------------
 const FileSpec &FileSpec::operator=(const FileSpec &rhs) {
   if (this != &rhs) {
     m_directory = rhs.m_directory;
     m_filename = rhs.m_filename;
void FileSpec::SetFile(llvm::StringRef pathname, bool resolve, Style style) {
 
   // Normalize back slashes to forward slashes
   if (m_style == Style::windows)
     std::replace(resolved.begin(), resolved.end(), '\\', '/');
 
   if (resolved.empty()) {
     // If we have no path after normalization set the path to the current
     // directory. This matches what python does and also a few other path
-    // utilities. 
+    // utilities.
     m_filename.SetString(".");
     return;
   }
 
-  m_filename.SetString(llvm::sys::path::filename(resolved, m_style));
-  llvm::StringRef dir = llvm::sys::path::parent_path(resolved, m_style);
-  if (!dir.empty())
-    m_directory.SetString(dir);
+  // Split path into filename and directory. We rely on the underlying char
+  // pointer to be nullptr when the components are empty.
+  llvm::StringRef filename = llvm::sys::path::filename(resolved, m_style);
+  if(!filename.empty())
+    m_filename.SetString(filename);
+  llvm::StringRef directory = llvm::sys::path::parent_path(resolved, m_style);
+  if(!directory.empty())
+    m_directory.SetString(directory);
 }
 
 void FileSpec::SetFile(llvm::StringRef path, bool resolve,
                        const llvm::Triple &Triple) {
   return SetFile(path, resolve,
                  Triple.isOSWindows() ? Style::windows : Style::posix);
 }
 
if (full || (a.m_directory && b.m_directory)) {
     result = ConstString::Compare(a.m_directory, b.m_directory, case_sensitive);
     if (result)
       return result;
   }
   return ConstString::Compare(a.m_filename, b.m_filename, case_sensitive);
 }
 
 bool FileSpec::Equal(const FileSpec &a, const FileSpec &b, bool full) {
-
   // case sensitivity of equality test
   const bool case_sensitive = a.IsCaseSensitive() || b.IsCaseSensitive();
-  
   const bool filenames_equal = ConstString::Equals(a.m_filename,
                                                    b.m_filename,
                                                    case_sensitive);
 
   if (!filenames_equal)
-      return false;
 
   if (!full && (a.GetDirectory().IsEmpty() || b.GetDirectory().IsEmpty()))
     return filenames_equal;
 
   return a == b;
 }
 
 //------------------------------------------------------------------
bool FileSpec::ResolveExecutableLocation() {
   return false;
 }
 
 bool FileSpec::ResolvePath() {
   if (m_is_resolved)
     return true; // We have already resolved this path
 
   // SetFile(...) will set m_is_resolved correctly if it can resolve the path
-  SetFile(GetPath(false), true);
+  SetFile(GetPath(false), true, m_style);
   return m_is_resolved;
 }
 
 uint64_t FileSpec::GetByteSize() const {
   uint64_t Size = 0;
   if (llvm::sys::fs::file_size(GetPath(), Size))
     return 0;
   return Size;
if (m_directory && m_filename && m_directory.GetStringRef().back() != '/' &&
     path.insert(path.end(), '/');
   path.append(m_filename.GetStringRef().begin(),
               m_filename.GetStringRef().end());
   if (denormalize && !path.empty())
     Denormalize(path, m_style);
 }
 
 ConstString FileSpec::GetFileNameExtension() const {
-  if (m_filename) {
-    const char *filename = m_filename.GetCString();
-    const char *dot_pos = strrchr(filename, '.');
-    if (dot_pos && dot_pos[1] != '\0')
-      return ConstString(dot_pos + 1);
-  }
-  return ConstString();
+  llvm::SmallString<64> current_path;
+  current_path.append(m_filename.GetStringRef().begin(),
+                      m_filename.GetStringRef().end());
+  return ConstString(llvm::sys::path::extension(current_path, m_style));
 }
 
 ConstString FileSpec::GetFileNameStrippingExtension() const {
-  const char *filename = m_filename.GetCString();
-  if (filename == NULL)
-    return ConstString();
-
-  const char *dot_pos = strrchr(filename, '.');
-  if (dot_pos == NULL)
-    return m_filename;
-
-  return ConstString(filename, dot_pos - filename);
+  llvm::SmallString<64> current_path;
+  current_path.append(m_filename.GetStringRef().begin(),
+                      m_filename.GetStringRef().end());
+  return ConstString(llvm::sys::path::stem(current_path, m_style));
 }
 
 //------------------------------------------------------------------
 // Return the size in bytes that this object takes in memory. This returns the
 // size in bytes of this object, not any shared string values it may refer to.
 //------------------------------------------------------------------
 size_t FileSpec::MemorySize() const {
   return m_filename.MemorySize() + m_directory.MemorySize();

 }
 
 void FileSpec::PrependPathComponent(llvm::StringRef component) {
   if (component.empty())
     return;
 
   const bool resolve = false;
   if (m_filename.IsEmpty() && m_directory.IsEmpty()) {
-    SetFile(component, resolve);
+    SetFile(component, resolve, m_style);
     return;
   }
 
   std::string result =
       join_path_components(m_style, {component, m_directory.GetStringRef(),
                                      m_filename.GetStringRef()});
   SetFile(result, resolve, m_style);
 }

 ///     file, \b false otherwise.
 //------------------------------------------------------------------
 bool FileSpec::IsSourceImplementationFile() const {
   ConstString extension(GetFileNameExtension());
   if (!extension)
     return false;
 
   static RegularExpression g_source_file_regex(llvm::StringRef(
-      "^([cC]|[mM]|[mM][mM]|[cC][pP][pP]|[cC]\\+\\+|[cC][xX][xX]|[cC][cC]|["
+      "^.([cC]|[mM]|[mM][mM]|[cC][pP][pP]|[cC]\\+\\+|[cC][xX][xX]|[cC][cC]|["
       "cC][pP]|[sS]|[aA][sS][mM]|[fF]|[fF]77|[fF]90|[fF]95|[fF]03|[fF][oO]["
       "rR]|[fF][tT][nN]|[fF][pP][pP]|[aA][dD][aA]|[aA][dD][bB]|[aA][dD][sS])"
       "$"));
   return g_source_file_regex.Execute(extension.GetStringRef());
 }
 
 bool FileSpec::IsRelative() const {
-  if (m_directory)
-    return PathIsRelative(m_directory.GetStringRef(), m_style);
-  else
-    return PathIsRelative(m_filename.GetStringRef(), m_style);
+  return !IsAbsolute();
 }
 
-bool FileSpec::IsAbsolute() const { return !FileSpec::IsRelative(); }
+bool FileSpec::IsAbsolute() const {
+  llvm::SmallString<64> current_path;
+  GetPath(current_path, false);
+
+  // Early return if the path is empty.
+  if (current_path.empty())
+    return false;
+
+  // We consider paths starting with ~ to be absolute.
+  if (current_path[0] == '~')
+    return true;
+
+  return llvm::sys::path::is_absolute(current_path, m_style);
+}
 
 void llvm::format_provider<FileSpec>::format(const FileSpec &F,
                                              raw_ostream &Stream,
                                              StringRef Style) {
   assert(
       (Style.empty() || Style.equals_lower("F") || Style.equals_lower("D")) &&
       "Invalid FileSpec style!");
 

lldb/trunk/unittests/Utility/FileSpecTest.cpp

TEST(FileSpecTest, FileAndDirectoryComponents) {
   // "F:/"
   EXPECT_STREQ("bar", fs_windows.GetFilename().GetCString());
 
   FileSpec fs_posix_root("/", false, FileSpec::Style::posix);
   EXPECT_STREQ("/", fs_posix_root.GetCString());
   EXPECT_EQ(nullptr, fs_posix_root.GetDirectory().GetCString());
   EXPECT_STREQ("/", fs_posix_root.GetFilename().GetCString());
 
+  FileSpec fs_net_drive("//net", false, FileSpec::Style::posix);
+  EXPECT_STREQ("//net", fs_net_drive.GetCString());
+  EXPECT_EQ(nullptr, fs_net_drive.GetDirectory().GetCString());
+  EXPECT_STREQ("//net", fs_net_drive.GetFilename().GetCString());
+
+  FileSpec fs_net_root("//net/", false, FileSpec::Style::posix);
+  EXPECT_STREQ("//net/", fs_net_root.GetCString());
+  EXPECT_STREQ("//net", fs_net_root.GetDirectory().GetCString());
+  EXPECT_STREQ("/", fs_net_root.GetFilename().GetCString());
+
   FileSpec fs_windows_drive("F:", false, FileSpec::Style::windows);
   EXPECT_STREQ("F:", fs_windows_drive.GetCString());
   EXPECT_EQ(nullptr, fs_windows_drive.GetDirectory().GetCString());
   EXPECT_STREQ("F:", fs_windows_drive.GetFilename().GetCString());
 
   FileSpec fs_windows_root("F:\\", false, FileSpec::Style::windows);
   EXPECT_STREQ("F:\\", fs_windows_root.GetCString());
   EXPECT_STREQ("F:", fs_windows_root.GetDirectory().GetCString());

 TEST(FileSpecTest, IsRelative) {
   llvm::StringRef not_relative[] = {
     "/",
     "/a",
     "/a/",
     "/a/b",
     "/a/b/",
     "//",
-    "//a",
     "//a/",
     "//a/b",
     "//a/b/",
     "~",
     "~/",
     "~/a",
     "~/a/",
     "~/a/b"