[FileSystem] Migrate CommandCompletions

[JDevlieghere]

Summary

Make use of the convenience helpers from FileSystem.

[davide]

No objections from me, but I would appreciate if @labath can take a look.

[JDevlieghere]

Initialize the FS in the unit test.

[labath]

Looks good, modulo the filesystem copy comment.

source/Commands/CommandCompletions.cpp

180 ↗

(On Diff #176525)

This should surely be a reference, no? Making a copy of a filesystem sounds weird (and should probably be forbidden by deleting the copy constructor).

[JDevlieghere]

source/Commands/CommandCompletions.cpp

180 ↗

(On Diff #176525)

Correct, this is a typo but shouldn't compile. I'll include the fix when landing.

lldb/trunk/include/lldb/Host/FileSystem.h

 class FileSystem {
 public:
   static const char *DEV_NULL;
   static const char *PATH_CONVERSION_ERROR;
 
   FileSystem() : m_fs(llvm::vfs::getRealFileSystem()) {}
   FileSystem(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs) : m_fs(fs) {}
 
+  FileSystem(const FileSystem &fs) = delete;
+  FileSystem &operator=(const FileSystem &fs) = delete;
+
   static FileSystem &Instance();
 
   static void Initialize();
   static void Initialize(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs);
   static void Terminate();
 
   Status Symlink(const FileSpec &src, const FileSpec &dst);
   Status Readlink(const FileSpec &src, FileSpec &dst);
 
   Status ResolveSymbolicLink(const FileSpec &src, FileSpec &dst);
 
   /// Wraps ::fopen in a platform-independent way.
   FILE *Fopen(const char *path, const char *mode);
 
   /// Wraps ::open in a platform-independent way.
   int Open(const char *path, int flags, int mode);
 
   Status Open(File &File, const FileSpec &file_spec, uint32_t options,
               uint32_t permissions = lldb::eFilePermissionsFileDefault);
 
+  /// Get a directory iterator.
+  /// @{
+  llvm::vfs::directory_iterator DirBegin(const FileSpec &file_spec,
+                                         std::error_code &ec);
+  llvm::vfs::directory_iterator DirBegin(const llvm::Twine &dir,
+                                         std::error_code &ec);
+  /// @}
+
+  /// Returns the Status object for the given file.
+  /// @{
+  llvm::ErrorOr<llvm::vfs::Status> GetStatus(const FileSpec &file_spec) const;
+  llvm::ErrorOr<llvm::vfs::Status> GetStatus(const llvm::Twine &path) const;
+  /// @}
+
   /// Returns the modification time of the given file.
   /// @{
   llvm::sys::TimePoint<> GetModificationTime(const FileSpec &file_spec) const;
   llvm::sys::TimePoint<> GetModificationTime(const llvm::Twine &path) const;
   /// @}
 
   /// Returns the on-disk size of the given file in bytes.
   /// @{

lldb/trunk/source/Commands/CommandCompletions.cpp

static int DiskFilesOrDirectories(const llvm::Twine &partial_name,
 
   llvm::SmallString<256> CompletionBuffer;
   llvm::SmallString<256> Storage;
   partial_name.toVector(CompletionBuffer);
 
   if (CompletionBuffer.size() >= PATH_MAX)
     return matches.GetSize();
 
-  namespace fs = llvm::sys::fs;
   namespace path = llvm::sys::path;
 
   llvm::StringRef SearchDir;
   llvm::StringRef PartialItem;
 
   if (CompletionBuffer.startswith("~")) {
     llvm::StringRef Buffer(CompletionBuffer);
     size_t FirstSep =
if (SearchDir.empty()) {
     llvm::sys::fs::current_path(Storage);
     SearchDir = Storage;
   }
   assert(!PartialItem.contains(path::get_separator()));
 
   // SearchDir now contains the directory to search in, and Prefix contains the
   // text we want to match against items in that directory.
 
+  FileSystem &fs = FileSystem::Instance();
   std::error_code EC;
-  fs::directory_iterator Iter(SearchDir, EC, false);
-  fs::directory_iterator End;
+  llvm::vfs::directory_iterator Iter = fs.DirBegin(SearchDir, EC);
+  llvm::vfs::directory_iterator End;
   for (; Iter != End && !EC; Iter.increment(EC)) {
     auto &Entry = *Iter;
+    llvm::ErrorOr<llvm::vfs::Status> Status = fs.GetStatus(Entry.path());
+
+    if (!Status)
+      continue;
 
     auto Name = path::filename(Entry.path());
 
     // Omit ".", ".."
     if (Name == "." || Name == ".." || !Name.startswith(PartialItem))
       continue;
 
-    // We have a match.
-
-    llvm::ErrorOr<fs::basic_file_status> st = Entry.status();
-    if (!st)
-      continue;
+    bool is_dir = Status->isDirectory();
 
     // If it's a symlink, then we treat it as a directory as long as the target
     // is a directory.
-    bool is_dir = fs::is_directory(*st);
-    if (fs::is_symlink_file(*st)) {
-      fs::file_status target_st;
-      if (!fs::status(Entry.path(), target_st))
-        is_dir = fs::is_directory(target_st);
+    if (Status->isSymlink()) {
+      FileSpec symlink_filespec(Entry.path());
+      FileSpec resolved_filespec;
+      auto error = fs.ResolveSymbolicLink(symlink_filespec, resolved_filespec);
+      if (error.Success())
+        is_dir = fs.IsDirectory(symlink_filespec);
     }
+
     if (only_directories && !is_dir)
       continue;
 
     // Shrink it back down so that it just has the original prefix the user
     // typed and remove the part of the name which is common to the located
     // item and what the user typed.
     CompletionBuffer.resize(FullPrefixLen);
     Name = Name.drop_front(PartialItem.size());

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

void FileSystem::Terminate() {
   InstanceImpl().reset();
 }
 
 Optional<FileSystem> &FileSystem::InstanceImpl() {
   static Optional<FileSystem> g_fs;
   return g_fs;
 }
 
+vfs::directory_iterator FileSystem::DirBegin(const FileSpec &file_spec,
+                                             std::error_code &ec) {
+  return DirBegin(file_spec.GetPath(), ec);
+}
+
+vfs::directory_iterator FileSystem::DirBegin(const Twine &dir,
+                                             std::error_code &ec) {
+  return m_fs->dir_begin(dir, ec);
+}
+
+llvm::ErrorOr<vfs::Status>
+FileSystem::GetStatus(const FileSpec &file_spec) const {
+  return GetStatus(file_spec.GetPath());
+}
+
+llvm::ErrorOr<vfs::Status> FileSystem::GetStatus(const Twine &path) const {
+  return m_fs->status(path);
+}
+
 sys::TimePoint<>
 FileSystem::GetModificationTime(const FileSpec &file_spec) const {
   return GetModificationTime(file_spec.GetPath());
 }
 
 sys::TimePoint<> FileSystem::GetModificationTime(const Twine &path) const {
   ErrorOr<vfs::Status> status = m_fs->status(path);
   if (!status)

lldb/trunk/unittests/Interpreter/TestCompletion.cpp

 //===-- TestCompletion.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/Host/FileSystem.h"
 #include "lldb/Interpreter/CommandCompletions.h"
 #include "lldb/Utility/StringList.h"
 #include "lldb/Utility/TildeExpressionResolver.h"
+
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include "TestingSupport/MockTildeExpressionResolver.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
protected:
   SmallString<128> FileAA;
   SmallString<128> FileAB;
   SmallString<128> FileAC;
   SmallString<128> FileFoo;
   SmallString<128> FileBar;
   SmallString<128> FileBaz;
 
   void SetUp() override {
+    FileSystem::Initialize();
+
     // chdir back into the original working dir this test binary started with.
     // A previous test may have have changed the working dir.
     ASSERT_NO_ERROR(fs::set_current_path(OriginalWorkingDir));
 
     // Get the name of the current test. To prevent that by chance two tests
     // get the same temporary directory if createUniqueDirectory fails.
     auto test_info = ::testing::UnitTest::GetInstance()->current_test_info();
     ASSERT_TRUE(test_info != nullptr);
for (auto File : llvm::zip(FileNames, Files)) {
       ::close(FD);
     }
   }
 
   static void SetUpTestCase() {
     ASSERT_NO_ERROR(fs::current_path(OriginalWorkingDir));
   }
 
-  void TearDown() override { ASSERT_NO_ERROR(fs::remove_directories(BaseDir)); }
+  void TearDown() override {
+    ASSERT_NO_ERROR(fs::remove_directories(BaseDir));
+    FileSystem::Terminate();
+  }
 
   static bool HasEquivalentFile(const Twine &Path, const StringList &Paths) {
     for (size_t I = 0; I < Paths.GetSize(); ++I) {
       if (fs::equivalent(Path, Paths[I]))
         return true;
     }
     return false;
   }
void DoDirCompletions(const Twine &Prefix,
     ASSERT_EQ(2u, Count);
     ASSERT_EQ(Count, Results.GetSize());
     EXPECT_TRUE(HasEquivalentFile(DirBar, Results));
     EXPECT_TRUE(HasEquivalentFile(DirBaz, Results));
   }
 };
 
 SmallString<128> CompletionTest::OriginalWorkingDir;
-}
+} // namespace
 
 static std::vector<std::string> toVector(const StringList &SL) {
   std::vector<std::string> Result;
   for (size_t Idx = 0; Idx < SL.GetSize(); ++Idx)
     Result.push_back(SL[Idx]);
   return Result;
 }
 using testing::UnorderedElementsAre;
TEST_F(CompletionTest, DirCompletionAbsolute) {
   // for that directory, not items under it.
   // Sanity check that the path we complete on exists and isn't too long.
   size_t Count = CommandCompletions::DiskDirectories(Twine(BaseDir) + "/fooa",
                                                      Results, Resolver);
   ASSERT_EQ(1u, Count);
   ASSERT_EQ(Count, Results.GetSize());
   EXPECT_TRUE(HasEquivalentFile(DirFooA, Results));
 
-  Count =
-    CommandCompletions::DiskDirectories(Twine(BaseDir) + "/.", Results, Resolver);
+  Count = CommandCompletions::DiskDirectories(Twine(BaseDir) + "/.", Results,
+                                              Resolver);
   ASSERT_EQ(0u, Count);
   ASSERT_EQ(Count, Results.GetSize());
 
   // When the same directory ends with a slash, it finds all children.
   Count = CommandCompletions::DiskDirectories(Prefixes[0], Results, Resolver);
   ASSERT_EQ(7u, Count);
   ASSERT_EQ(Count, Results.GetSize());
   EXPECT_TRUE(HasEquivalentFile(DirFoo, Results));