[lldb] Introduce FileSpec::GetComponents

[bulbazord]

Summary

This patch introduces FileSpec::GetComponents, a method that splits a
FileSpec's path into its individual components. For example, given
/foo/bar/baz, you'll get back a vector of strings {"foo", "bar", baz"}.

The motivation here is to reduce the use of
FileSpec::RemoveLastPathComponent. Mutating a FileSpec is expensive,
so providing a way of doing this without mutation is useful.

[JDevlieghere]

lldb/include/lldb/Utility/FileSpec.h

420

I'm surprised this returns a vector of std::strings and not llvm::StringRefs. I would expect all the components to be part of the FileSpec's stored path. Even with the file and directory stored as separate ConstStrings, that should be possible?

[bulbazord]

lldb/include/lldb/Utility/FileSpec.h

420

Yes, it is possible to do std::vector<llvm::StringRef> here, especially because they would be backed by ConstStrings which live forever. I chose std::string here because of the possibility that we one day no longer use ConstString to store the path, in which case the vector's StringRefs would only be valid for the lifetime of the FileSpec (or until it gets mutated).

Maybe I'm thinking too far ahead or planning for a future that will never come though. What do you think?

[JDevlieghere]

lldb/include/lldb/Utility/FileSpec.h

420

I think it's reasonable to expect the lifetime of things handed out by a FileSpec match the lifetime of the FileSpec, but that depends on how we want to deal with mutability. If we want to be able to mutate a FileSpec in place, then you're right, these things need to have their own lifetime.

[bulbazord]

lldb/include/lldb/Utility/FileSpec.h

420

I'd prefer to move towards FileSpec being immutable (or as close as possible), so I'll update this and change it to `StringRef. Thanks for the feedback!

[bulbazord]

Convert return type to std::vector<llvm::StringRef>
Actually fix code so this thing compiles correctly

[mib]

LGTM!

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp

1241

Cool!

lldb/include/lldb/Utility/FileSpec.h

public:
 
   /// Removes the last path component by replacing the current path with its
   /// parent. When the current path has no parent, this is a no-op.
   ///
   /// \return
   ///     A boolean value indicating whether the path was updated.
   bool RemoveLastPathComponent();
 
+  /// Gets the components of the FileSpec's path.
+  /// For example, given the path:
+  ///   /System/Library/PrivateFrameworks/UIFoundation.framework/UIFoundation
+  ///
+  /// This function returns:
+  ///   {"System", "Library", "PrivateFrameworks", "UIFoundation.framework",
+  ///   "UIFoundation"}
+  /// \return
+  ///   A std::vector of std::strings for each path component.
+  std::vector<std::string> GetComponents() const;
+
 protected:
   // Convenience method for setting the file without changing the style.
   void SetFile(llvm::StringRef path);
 
   /// Called anytime m_directory or m_filename is changed to clear any cached
   /// state in this object.
   void PathWasModified() {
     m_is_resolved = false;

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp

if (!module_sp && module_search_paths_ptr && platform_file) {
     // create a vector of all the file / directory names in platform_file e.g.
     // this might be
     // /System/Library/PrivateFrameworks/UIFoundation.framework/UIFoundation
     //
     // We'll need to look in the module_search_paths_ptr directories for both
     // "UIFoundation" and "UIFoundation.framework" -- most likely the latter
     // will be the one we find there.
 
-    FileSpec platform_pull_upart(platform_file);
-    std::vector<std::string> path_parts;
-    path_parts.push_back(platform_pull_upart.GetFilename().AsCString());
-    while (platform_pull_upart.RemoveLastPathComponent()) {
-      ConstString part = platform_pull_upart.GetFilename();
-      path_parts.push_back(part.AsCString());
-    }
+    std::vector<std::string> path_components = platform_file.GetComponents();
+    // We want the components in reverse order.
+    std::reverse(path_components.begin(), path_components.end());
     const size_t path_parts_size = path_parts.size();
 
     size_t num_module_search_paths = module_search_paths_ptr->GetSize();
     for (size_t i = 0; i < num_module_search_paths; ++i) {
       Log *log_verbose = GetLog(LLDBLog::Host);
       LLDB_LOGF(
           log_verbose,
           "PlatformRemoteDarwinDevice::GetSharedModule searching for binary in "

lldb/source/Utility/FileSpec.cpp

bool FileSpec::RemoveLastPathComponent() {
   llvm::SmallString<64> current_path;
   GetPath(current_path, false);
   if (llvm::sys::path::has_parent_path(current_path, m_style)) {
     SetFile(llvm::sys::path::parent_path(current_path, m_style));
     return true;
   }
   return false;
 }
+
+std::vector<std::string> FileSpec::GetComponents() const {
+  std::vector<std::string> components;
+
+  llvm::SmallString<64> current_path;
+  GetPath(current_path, false);
+
+  auto begin = llvm::sys::path::begin(current_path, m_style);
+  auto end = llvm::sys::path::end(current_path);
+
+  for (auto iter = begin; iter != end; ++iter) {
+    if (*iter == "/" || *iter == ".")
+      continue;
+
+    components.push_back(iter->str());
+  }
+
+  return components;
+}
+
 /// Returns true if the filespec represents an implementation source
 /// file (files with a ".c", ".cpp", ".m", ".mm" (many more)
 /// extension).
 ///
 /// \return
 ///     \b true if the filespec represents an implementation source
 ///     file, \b false otherwise.
 bool FileSpec::IsSourceImplementationFile() const {

lldb/unittests/Utility/FileSpecTest.cpp

TEST(FileSpecTest, TestIsSourceImplementationFile) {
   FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
   FileSpec exe = WindowsSpec("C:\\tmp\\foo.exe");
 
   EXPECT_TRUE(cpp_src.IsSourceImplementationFile());
   EXPECT_FALSE(dll.IsSourceImplementationFile());
   EXPECT_FALSE(win_noext.IsSourceImplementationFile());
   EXPECT_FALSE(exe.IsSourceImplementationFile());
 }
+
+TEST(FileSpecTest, TestGetComponents) {
+  std::pair<llvm::StringRef, std::vector<std::string>> PosixTests[] = {
+      {"/", {}},
+      {"/foo", {"foo"}},
+      {"/foo/", {"foo"}},
+      {"/foo/bar", {"foo", "bar"}},
+      {"/llvm-project/lldb/unittests/Utility/FileSpecTest.cpp",
+       {"llvm-project", "lldb", "unittests", "Utility", "FileSpecTest.cpp"}},
+  };
+
+  for (const auto &pair : PosixTests) {
+    FileSpec file_spec = PosixSpec(pair.first);
+    EXPECT_EQ(file_spec.GetComponents(), pair.second);
+  }
+
+  std::pair<llvm::StringRef, std::vector<std::string>> WindowsTests[] = {
+      {"C:\\", {"C:"}},
+      {"C:\\Windows\\", {"C:", "Windows"}},
+      {"C:\\Windows\\System32", {"C:", "Windows", "System32"}},
+      {"C:\\llvm-project\\lldb\\unittests\\Utility\\FileSpecTest.cpp",
+       {"C:", "llvm-project", "lldb", "unittests", "Utility",
+        "FileSpecTest.cpp"}},
+  };
+
+  for (const auto &pair : WindowsTests) {
+    FileSpec file_spec = WindowsSpec(pair.first);
+    EXPECT_EQ(file_spec.GetComponents(), pair.second);
+  }
+}