[PATCH] D78629: [clangd] Look for compilation database in `build` subdirectory of parents.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG226b045b1fe5: [clangd] Look for compilation database in 
`build` subdirectory of parents. (authored by sammccall).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78629/new/

https://reviews.llvm.org/D78629

Files:
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp

Index: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
===
--- clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
+++ clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
@@ -8,6 +8,7 @@
 
 #include "GlobalCompilationDatabase.h"
 
+#include "Matchers.h"
 #include "Path.h"
 #include "TestFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
@@ -164,6 +165,47 @@
"-DFallback", "-DAdjust_baz.cc"));
 }
 
+// Allows placement of files for tests and cleans them up after.
+class ScratchFS {
+  llvm::SmallString<128> Root;
+
+public:
+  ScratchFS() {
+EXPECT_FALSE(llvm::sys::fs::createUniqueDirectory("clangd-cdb-test", Root))
+<< "Failed to create unique directory";
+  }
+
+  ~ScratchFS() {
+EXPECT_FALSE(llvm::sys::fs::remove_directories(Root))
+<< "Failed to cleanup " << Root;
+  }
+
+  llvm::StringRef root() const { return Root; }
+
+  void write(PathRef RelativePath, llvm::StringRef Contents) {
+std::string AbsPath = path(RelativePath);
+EXPECT_FALSE(llvm::sys::fs::create_directories(
+llvm::sys::path::parent_path(AbsPath)))
+<< "Failed to create directories for: " << AbsPath;
+
+std::error_code EC;
+llvm::raw_fd_ostream OS(AbsPath, EC);
+EXPECT_FALSE(EC) << "Failed to open " << AbsPath << " for writing";
+OS << llvm::formatv(Contents.data(),
+llvm::sys::path::convert_to_slash(Root));
+OS.close();
+
+EXPECT_FALSE(OS.has_error());
+  }
+
+  std::string path(PathRef RelativePath) const {
+llvm::SmallString<128> AbsPath(Root);
+llvm::sys::path::append(AbsPath, RelativePath);
+llvm::sys::path::native(AbsPath);
+return AbsPath.str().str();
+  }
+};
+
 TEST(GlobalCompilationDatabaseTest, DiscoveryWithNestedCDBs) {
   const char *const CDBOuter =
   R"cdb(
@@ -195,44 +237,9 @@
 }
   ]
   )cdb";
-  class CleaningFS {
-  public:
-llvm::SmallString<128> Root;
-
-CleaningFS() {
-  EXPECT_FALSE(
-  llvm::sys::fs::createUniqueDirectory("clangd-cdb-test", Root))
-  << "Failed to create unique directory";
-}
-
-~CleaningFS() {
-  EXPECT_FALSE(llvm::sys::fs::remove_directories(Root))
-  << "Failed to cleanup " << Root;
-}
-
-void registerFile(PathRef RelativePath, llvm::StringRef Contents) {
-  llvm::SmallString<128> AbsPath(Root);
-  llvm::sys::path::append(AbsPath, RelativePath);
-
-  EXPECT_FALSE(llvm::sys::fs::create_directories(
-  llvm::sys::path::parent_path(AbsPath)))
-  << "Failed to create directories for: " << AbsPath;
-
-  std::error_code EC;
-  llvm::raw_fd_ostream OS(AbsPath, EC);
-  EXPECT_FALSE(EC) << "Failed to open " << AbsPath << " for writing";
-  OS << llvm::formatv(Contents.data(),
-  llvm::sys::path::convert_to_slash(Root));
-  OS.close();
-
-  EXPECT_FALSE(OS.has_error());
-}
-  };
-
-  CleaningFS FS;
-  FS.registerFile("compile_commands.json", CDBOuter);
-  FS.registerFile("build/compile_commands.json", CDBInner);
-  llvm::SmallString<128> File;
+  ScratchFS FS;
+  FS.write("compile_commands.json", CDBOuter);
+  FS.write("build/compile_commands.json", CDBInner);
 
   // Note that gen2.cc goes missing with our following model, not sure this
   // happens in practice though.
@@ -244,43 +251,50 @@
   DiscoveredFiles = Changes;
 });
 
-File = FS.Root;
-llvm::sys::path::append(File, "build", "..", "a.cc");
-DB.getCompileCommand(File.str());
+DB.getCompileCommand(FS.path("build/../a.cc"));
 EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(AllOf(
  EndsWith("a.cc"), Not(HasSubstr("..");
 DiscoveredFiles.clear();
 
-File = FS.Root;
-llvm::sys::path::append(File, "build", "gen.cc");
-DB.getCompileCommand(File.str());
+DB.getCompileCommand(FS.path("build/gen.cc"));
 EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(EndsWith("gen.cc")));
   }
 
   // With a custom compile commands dir.
   {
-DirectoryBasedGlobalCompilationDatabase DB(FS.Root.str().str());
+DirectoryBasedGlobalCompilationDatabase DB(FS.root().str());
 std::vector DiscoveredFiles;
 auto Sub =
 DB.watch([](const std::vector Changes) {
   DiscoveredFiles = Changes;
 });
 
-File = FS.Root;
-

[PATCH] D78629: [clangd] Look for compilation database in `build` subdirectory of parents.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

sorry this fell of my radar. LGTM.

As discussed offline i am a little bit worried about regressing the case when 
there's  a `build` in a subdirectory, and a (merged) compilation database of 
some sort in a parent directory. But I suppose this is a rare-ish use case.

now I can delete my symlink :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78629/new/

https://reviews.llvm.org/D78629



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78629: [clangd] Look for compilation database in `build` subdirectory of parents.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

This matches the conventional location of cmake build directories.
It also allows creating a `build` symlink, which seems more flexible than the
compile_commands.json symlinks.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78629

Files:
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp

Index: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
===
--- clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
+++ clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
@@ -8,6 +8,7 @@
 
 #include "GlobalCompilationDatabase.h"
 
+#include "Matchers.h"
 #include "Path.h"
 #include "TestFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
@@ -164,6 +165,47 @@
"-DFallback", "-DAdjust_baz.cc"));
 }
 
+// Allows placement of files for tests and cleans them up after.
+class ScratchFS {
+  llvm::SmallString<128> Root;
+
+public:
+  ScratchFS() {
+EXPECT_FALSE(llvm::sys::fs::createUniqueDirectory("clangd-cdb-test", Root))
+<< "Failed to create unique directory";
+  }
+
+  ~ScratchFS() {
+EXPECT_FALSE(llvm::sys::fs::remove_directories(Root))
+<< "Failed to cleanup " << Root;
+  }
+
+  llvm::StringRef root() const { return Root; }
+
+  void write(PathRef RelativePath, llvm::StringRef Contents) {
+std::string AbsPath = path(RelativePath);
+EXPECT_FALSE(llvm::sys::fs::create_directories(
+llvm::sys::path::parent_path(AbsPath)))
+<< "Failed to create directories for: " << AbsPath;
+
+std::error_code EC;
+llvm::raw_fd_ostream OS(AbsPath, EC);
+EXPECT_FALSE(EC) << "Failed to open " << AbsPath << " for writing";
+OS << llvm::formatv(Contents.data(),
+llvm::sys::path::convert_to_slash(Root));
+OS.close();
+
+EXPECT_FALSE(OS.has_error());
+  }
+
+  std::string path(PathRef RelativePath) const {
+llvm::SmallString<128> AbsPath(Root);
+llvm::sys::path::append(AbsPath, RelativePath);
+llvm::sys::path::native(AbsPath);
+return AbsPath.str().str();
+  }
+};
+
 TEST(GlobalCompilationDatabaseTest, DiscoveryWithNestedCDBs) {
   const char *const CDBOuter =
   R"cdb(
@@ -195,44 +237,9 @@
 }
   ]
   )cdb";
-  class CleaningFS {
-  public:
-llvm::SmallString<128> Root;
-
-CleaningFS() {
-  EXPECT_FALSE(
-  llvm::sys::fs::createUniqueDirectory("clangd-cdb-test", Root))
-  << "Failed to create unique directory";
-}
-
-~CleaningFS() {
-  EXPECT_FALSE(llvm::sys::fs::remove_directories(Root))
-  << "Failed to cleanup " << Root;
-}
-
-void registerFile(PathRef RelativePath, llvm::StringRef Contents) {
-  llvm::SmallString<128> AbsPath(Root);
-  llvm::sys::path::append(AbsPath, RelativePath);
-
-  EXPECT_FALSE(llvm::sys::fs::create_directories(
-  llvm::sys::path::parent_path(AbsPath)))
-  << "Failed to create directories for: " << AbsPath;
-
-  std::error_code EC;
-  llvm::raw_fd_ostream OS(AbsPath, EC);
-  EXPECT_FALSE(EC) << "Failed to open " << AbsPath << " for writing";
-  OS << llvm::formatv(Contents.data(),
-  llvm::sys::path::convert_to_slash(Root));
-  OS.close();
-
-  EXPECT_FALSE(OS.has_error());
-}
-  };
-
-  CleaningFS FS;
-  FS.registerFile("compile_commands.json", CDBOuter);
-  FS.registerFile("build/compile_commands.json", CDBInner);
-  llvm::SmallString<128> File;
+  ScratchFS FS;
+  FS.write("compile_commands.json", CDBOuter);
+  FS.write("build/compile_commands.json", CDBInner);
 
   // Note that gen2.cc goes missing with our following model, not sure this
   // happens in practice though.
@@ -244,43 +251,50 @@
   DiscoveredFiles = Changes;
 });
 
-File = FS.Root;
-llvm::sys::path::append(File, "build", "..", "a.cc");
-DB.getCompileCommand(File.str());
+DB.getCompileCommand(FS.path("build/../a.cc"));
 EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(AllOf(
  EndsWith("a.cc"), Not(HasSubstr("..");
 DiscoveredFiles.clear();
 
-File = FS.Root;
-llvm::sys::path::append(File, "build", "gen.cc");
-DB.getCompileCommand(File.str());
+DB.getCompileCommand(FS.path("build/gen.cc"));
 EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(EndsWith("gen.cc")));
   }
 
   // With a custom compile commands dir.
   {
-DirectoryBasedGlobalCompilationDatabase DB(FS.Root.str().str());
+DirectoryBasedGlobalCompilationDatabase DB(FS.root().str());
 std::vector DiscoveredFiles;
 auto Sub =
 DB.watch([](const std::vector