[PATCH] D64247: [clangd] Filter out non-governed files from broadcast

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365634: [clangd] Filter out non-governed files from 
broadcast (authored by kadircet, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64247?vs=208890=208957#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64247

Files:
  clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
  clang-tools-extra/trunk/clangd/QueryDriverDatabase.cpp
  clang-tools-extra/trunk/clangd/index/Background.cpp
  clang-tools-extra/trunk/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/trunk/clangd/unittests/GlobalCompilationDatabaseTests.cpp
  clang-tools-extra/trunk/clangd/unittests/TestFS.cpp
  clang-tools-extra/trunk/clangd/unittests/TestFS.h

Index: clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
===
--- clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
+++ clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
@@ -40,9 +40,13 @@
   virtual ~GlobalCompilationDatabase() = default;
 
   /// If there are any known-good commands for building this file, returns one.
-  /// If the ProjectInfo pointer is set, it will also be populated.
   virtual llvm::Optional
-  getCompileCommand(PathRef File, ProjectInfo * = nullptr) const = 0;
+  getCompileCommand(PathRef File) const = 0;
+
+  /// Finds the closest project to \p File.
+  virtual llvm::Optional getProjectInfo(PathRef File) const {
+return llvm::None;
+  }
 
   /// Makes a guess at how to build a file.
   /// The default implementation just runs clang on the file.
@@ -71,20 +75,40 @@
 
   /// Scans File's parents looking for compilation databases.
   /// Any extra flags will be added.
+  /// Might trigger OnCommandChanged, if CDB wasn't broadcasted yet.
   llvm::Optional
-  getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override;
+  getCompileCommand(PathRef File) const override;
+
+  /// Returns the path to first directory containing a compilation database in
+  /// \p File's parents.
+  llvm::Optional getProjectInfo(PathRef File) const override;
 
 private:
-  tooling::CompilationDatabase *getCDBForFile(PathRef File,
-  ProjectInfo *) const;
-  std::pair
+  std::pair
   getCDBInDirLocked(PathRef File) const;
 
+  struct CDBLookupRequest {
+PathRef FileName;
+// Whether this lookup should trigger discovery of the CDB found.
+bool ShouldBroadcast = false;
+  };
+  struct CDBLookupResult {
+tooling::CompilationDatabase *CDB = nullptr;
+ProjectInfo PI;
+  };
+  llvm::Optional lookupCDB(CDBLookupRequest Request) const;
+
+  // Performs broadcast on governed files.
+  void broadcastCDB(CDBLookupResult Res) const;
+
   mutable std::mutex Mutex;
   /// Caches compilation databases loaded from directories(keys are
   /// directories).
-  mutable llvm::StringMap>
-  CompilationDatabases;
+  struct CachedCDB {
+std::unique_ptr CDB = nullptr;
+bool SentBroadcast = false;
+  };
+  mutable llvm::StringMap CompilationDatabases;
 
   /// Used for command argument pointing to folder where compile_commands.json
   /// is located.
@@ -109,8 +133,9 @@
  llvm::Optional ResourceDir = llvm::None);
 
   llvm::Optional
-  getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override;
+  getCompileCommand(PathRef File) const override;
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
+  llvm::Optional getProjectInfo(PathRef File) const override;
 
   /// Sets or clears the compilation command for a particular file.
   void
Index: clang-tools-extra/trunk/clangd/unittests/TestFS.h
===
--- clang-tools-extra/trunk/clangd/unittests/TestFS.h
+++ clang-tools-extra/trunk/clangd/unittests/TestFS.h
@@ -12,6 +12,8 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
 #include "ClangdServer.h"
+#include "GlobalCompilationDatabase.h"
+#include "Path.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
@@ -48,7 +50,9 @@
   StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
-  getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override;
+  getCompileCommand(PathRef File) const override;
+
+  llvm::Optional getProjectInfo(PathRef File) const override;
 
   std::vector ExtraClangFlags;
 
Index: clang-tools-extra/trunk/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/ClangdTests.cpp
+++ 

[PATCH] D64247: [clangd] Filter out non-governed files from broadcast

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 208890.
kadircet marked 3 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64247

Files:
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/GlobalCompilationDatabase.h
  clang-tools-extra/clangd/QueryDriverDatabase.cpp
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
  clang-tools-extra/clangd/unittests/TestFS.cpp
  clang-tools-extra/clangd/unittests/TestFS.h

Index: clang-tools-extra/clangd/unittests/TestFS.h
===
--- clang-tools-extra/clangd/unittests/TestFS.h
+++ clang-tools-extra/clangd/unittests/TestFS.h
@@ -12,6 +12,8 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
 #include "ClangdServer.h"
+#include "GlobalCompilationDatabase.h"
+#include "Path.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
@@ -48,7 +50,9 @@
   StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
-  getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override;
+  getCompileCommand(PathRef File) const override;
+
+  llvm::Optional getProjectInfo(PathRef File) const override;
 
   std::vector ExtraClangFlags;
 
Index: clang-tools-extra/clangd/unittests/TestFS.cpp
===
--- clang-tools-extra/clangd/unittests/TestFS.cpp
+++ clang-tools-extra/clangd/unittests/TestFS.cpp
@@ -6,7 +6,11 @@
 //
 //===--===//
 #include "TestFS.h"
+#include "GlobalCompilationDatabase.h"
+#include "Path.h"
 #include "URI.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
@@ -36,9 +40,13 @@
   // -ffreestanding avoids implicit stdc-predef.h.
 }
 
+llvm::Optional
+MockCompilationDatabase::getProjectInfo(PathRef File) const {
+  return ProjectInfo{Directory};
+};
+
 llvm::Optional
-MockCompilationDatabase::getCompileCommand(PathRef File,
-   ProjectInfo *Project) const {
+MockCompilationDatabase::getCompileCommand(PathRef File) const {
   if (ExtraClangFlags.empty())
 return None;
 
@@ -57,8 +65,6 @@
 CommandLine.push_back(RelativeFilePath.str());
   }
 
-  if (Project)
-Project->SourceRoot = Directory;
   return {tooling::CompileCommand(Directory != llvm::StringRef()
   ? Directory
   : llvm::sys::path::parent_path(File),
Index: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
===
--- clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
+++ clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
@@ -8,10 +8,21 @@
 
 #include "GlobalCompilationDatabase.h"
 
+#include "Path.h"
 #include "TestFS.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -20,8 +31,10 @@
 using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::EndsWith;
+using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::StartsWith;
+using ::testing::UnorderedElementsAre;
 
 TEST(GlobalCompilationDatabaseTest, FallbackCommand) {
   DirectoryBasedGlobalCompilationDatabase DB(None);
@@ -50,13 +63,9 @@
   class BaseCDB : public GlobalCompilationDatabase {
   public:
 llvm::Optional
-getCompileCommand(llvm::StringRef File,
-  ProjectInfo *Project) const override {
-  if (File == testPath("foo.cc")) {
-if (Project)
-  Project->SourceRoot = testRoot();
+getCompileCommand(llvm::StringRef File) const override {
+  if (File == testPath("foo.cc"))
 return cmd(File, "-DA=1");
-  }
   return None;
 }
 
@@ -64,6 +73,10 @@
 getFallbackCommand(llvm::StringRef File) const override {
   return cmd(File, "-DA=2");
 }
+
+llvm::Optional getProjectInfo(PathRef File) const override {
+  return ProjectInfo{testRoot()};
+}
   };
 
 protected:
@@ -153,6 +166,109 @@
 Not(Contains("random-plugin";
 }
 

[PATCH] D64247: [clangd] Filter out non-governed files from broadcast

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:118
+  DirectoryHasCDB[Path] = false;
+  return false;
+});

return Path == Result.PI.SourceRoot?



Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:125
+std::lock_guard Lock(Mutex);
+for (auto  : DirectoryHasCDB) {
+  auto Res = getCDBInDirLocked(Entry.first());

(Why not do this in the one pass above? Doesn't seem worth two passes to avoid 
locking while iterating over AllFiles)



Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:134
+  for (llvm::StringRef File : AllFiles) {
+// This check assumes that a source file is governed by the closest
+// compilation database. Independent of whether it has an entry for that

assumes isn't strong enough here - we're defining it this way, and should say 
why.

e.g. "A file is only part of this CDB if lookup for the file would find this 
CDB."



Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:183
+
+  // FIXME: Maybe make the following part async, since this can block retrieval
+  // of compile commands.

I don't think we'll ever fix this, feel free to drop the FIXME if you like.



Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.h:115
   /// is located.
   llvm::Optional CompileCommandsDir;
 };

(random note: we should really rename this something like FixedCDBDir. Probably 
not in this patch, but what a confusing name)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64247



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

[PATCH] D64247: [clangd] Filter out non-governed files from broadcast

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 208697.
kadircet added a comment.

- Rearrange changes to get a better diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64247

Files:
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/GlobalCompilationDatabase.h
  clang-tools-extra/clangd/QueryDriverDatabase.cpp
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
  clang-tools-extra/clangd/unittests/TestFS.cpp
  clang-tools-extra/clangd/unittests/TestFS.h

Index: clang-tools-extra/clangd/unittests/TestFS.h
===
--- clang-tools-extra/clangd/unittests/TestFS.h
+++ clang-tools-extra/clangd/unittests/TestFS.h
@@ -12,6 +12,8 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
 #include "ClangdServer.h"
+#include "GlobalCompilationDatabase.h"
+#include "Path.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
@@ -48,7 +50,9 @@
   StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
-  getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override;
+  getCompileCommand(PathRef File) const override;
+
+  llvm::Optional getProjectInfo(PathRef File) const override;
 
   std::vector ExtraClangFlags;
 
Index: clang-tools-extra/clangd/unittests/TestFS.cpp
===
--- clang-tools-extra/clangd/unittests/TestFS.cpp
+++ clang-tools-extra/clangd/unittests/TestFS.cpp
@@ -6,7 +6,11 @@
 //
 //===--===//
 #include "TestFS.h"
+#include "GlobalCompilationDatabase.h"
+#include "Path.h"
 #include "URI.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
@@ -36,9 +40,13 @@
   // -ffreestanding avoids implicit stdc-predef.h.
 }
 
+llvm::Optional
+MockCompilationDatabase::getProjectInfo(PathRef File) const {
+  return ProjectInfo{Directory};
+};
+
 llvm::Optional
-MockCompilationDatabase::getCompileCommand(PathRef File,
-   ProjectInfo *Project) const {
+MockCompilationDatabase::getCompileCommand(PathRef File) const {
   if (ExtraClangFlags.empty())
 return None;
 
@@ -57,8 +65,6 @@
 CommandLine.push_back(RelativeFilePath.str());
   }
 
-  if (Project)
-Project->SourceRoot = Directory;
   return {tooling::CompileCommand(Directory != llvm::StringRef()
   ? Directory
   : llvm::sys::path::parent_path(File),
Index: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
===
--- clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
+++ clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
@@ -8,10 +8,21 @@
 
 #include "GlobalCompilationDatabase.h"
 
+#include "Path.h"
 #include "TestFS.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -20,8 +31,10 @@
 using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::EndsWith;
+using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::StartsWith;
+using ::testing::UnorderedElementsAre;
 
 TEST(GlobalCompilationDatabaseTest, FallbackCommand) {
   DirectoryBasedGlobalCompilationDatabase DB(None);
@@ -50,13 +63,9 @@
   class BaseCDB : public GlobalCompilationDatabase {
   public:
 llvm::Optional
-getCompileCommand(llvm::StringRef File,
-  ProjectInfo *Project) const override {
-  if (File == testPath("foo.cc")) {
-if (Project)
-  Project->SourceRoot = testRoot();
+getCompileCommand(llvm::StringRef File) const override {
+  if (File == testPath("foo.cc"))
 return cmd(File, "-DA=1");
-  }
   return None;
 }
 
@@ -64,6 +73,10 @@
 getFallbackCommand(llvm::StringRef File) const override {
   return cmd(File, "-DA=2");
 }
+
+llvm::Optional getProjectInfo(PathRef File) const override {
+  return ProjectInfo{testRoot()};
+}
   };
 
 protected:
@@ -153,6 +166,109 @@
 Not(Contains("random-plugin";
 }
 
+TEST(GlobalCompilationDatabaseTest, 

[PATCH] D64247: [clangd] Filter out non-governed files from broadcast

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/index/Background.cpp:653
+auto PI = CDB.getProjectInfo(File);
+assert(PI && "Found CDB but no ProjectInfo!");
+

sammccall wrote:
> This looks like a bad assertion: it should be OK to provide compile commands 
> but not project info.
> 
> (Otherwise getProjectInfo should be pure virtual, but I'm concerned about the 
> raciness if we're going to insist they're consistent but not provide any way 
> of synchronizing)
> 
> I'd suggest we should just not index such files (for now). Later we can start 
> passing "" to the index storage factory to get the fallback storage (I think 
> today we pass "", but the factory doesn't handle it - oops!)
actually we have a nullstorage for that, which just logs a messsage and doesn't 
persist the shard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64247



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

[PATCH] D64247: [clangd] Filter out non-governed files from broadcast

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 208686.
kadircet marked 3 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64247

Files:
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/GlobalCompilationDatabase.h
  clang-tools-extra/clangd/QueryDriverDatabase.cpp
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
  clang-tools-extra/clangd/unittests/TestFS.cpp
  clang-tools-extra/clangd/unittests/TestFS.h

Index: clang-tools-extra/clangd/unittests/TestFS.h
===
--- clang-tools-extra/clangd/unittests/TestFS.h
+++ clang-tools-extra/clangd/unittests/TestFS.h
@@ -12,6 +12,8 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
 #include "ClangdServer.h"
+#include "GlobalCompilationDatabase.h"
+#include "Path.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
@@ -48,7 +50,9 @@
   StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
-  getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override;
+  getCompileCommand(PathRef File) const override;
+
+  llvm::Optional getProjectInfo(PathRef File) const override;
 
   std::vector ExtraClangFlags;
 
Index: clang-tools-extra/clangd/unittests/TestFS.cpp
===
--- clang-tools-extra/clangd/unittests/TestFS.cpp
+++ clang-tools-extra/clangd/unittests/TestFS.cpp
@@ -6,7 +6,11 @@
 //
 //===--===//
 #include "TestFS.h"
+#include "GlobalCompilationDatabase.h"
+#include "Path.h"
 #include "URI.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
@@ -36,9 +40,13 @@
   // -ffreestanding avoids implicit stdc-predef.h.
 }
 
+llvm::Optional
+MockCompilationDatabase::getProjectInfo(PathRef File) const {
+  return ProjectInfo{Directory};
+};
+
 llvm::Optional
-MockCompilationDatabase::getCompileCommand(PathRef File,
-   ProjectInfo *Project) const {
+MockCompilationDatabase::getCompileCommand(PathRef File) const {
   if (ExtraClangFlags.empty())
 return None;
 
@@ -57,8 +65,6 @@
 CommandLine.push_back(RelativeFilePath.str());
   }
 
-  if (Project)
-Project->SourceRoot = Directory;
   return {tooling::CompileCommand(Directory != llvm::StringRef()
   ? Directory
   : llvm::sys::path::parent_path(File),
Index: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
===
--- clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
+++ clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
@@ -8,10 +8,21 @@
 
 #include "GlobalCompilationDatabase.h"
 
+#include "Path.h"
 #include "TestFS.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -20,8 +31,10 @@
 using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::EndsWith;
+using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::StartsWith;
+using ::testing::UnorderedElementsAre;
 
 TEST(GlobalCompilationDatabaseTest, FallbackCommand) {
   DirectoryBasedGlobalCompilationDatabase DB(None);
@@ -50,13 +63,9 @@
   class BaseCDB : public GlobalCompilationDatabase {
   public:
 llvm::Optional
-getCompileCommand(llvm::StringRef File,
-  ProjectInfo *Project) const override {
-  if (File == testPath("foo.cc")) {
-if (Project)
-  Project->SourceRoot = testRoot();
+getCompileCommand(llvm::StringRef File) const override {
+  if (File == testPath("foo.cc"))
 return cmd(File, "-DA=1");
-  }
   return None;
 }
 
@@ -64,6 +73,10 @@
 getFallbackCommand(llvm::StringRef File) const override {
   return cmd(File, "-DA=2");
 }
+
+llvm::Optional getProjectInfo(PathRef File) const override {
+  return ProjectInfo{testRoot()};
+}
   };
 
 protected:
@@ -153,6 +166,109 @@
 Not(Contains("random-plugin";
 }
 

[PATCH] D64247: [clangd] Filter out non-governed files from broadcast

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:94
+
+  // It doesn't make sense to perform a traversal if user said their 
compilation
+  // database is in a different place. Return that directory for all files.

The duplication here is a bit troublesome.

And we have divergences in behavior - probably not important ones, but I can't 
say I know for sure. (e.g. here we won't create the CDB as a side-effect of 
getPathInfo() if there's a CompileCommandsDir, and will return the dir even if 
the CDB doesn't exist)

I think we might want to have some central logic parameterized by a struct, e.g.
```
struct DirectoryBasedCompilationDatabase::Lookup {
  // inputs
  PathRef File;
  bool ShouldBroadcast;

  // outputs
  tooling::CompilationDatabase *CDB;
  ProjectInfo Info;
  bool EverBroadcast;
};
lookupCDB(Lookup); // replaces getCDBInDirLocked
```



Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:176
+std::vector GovernedFiles;
+for (llvm::StringRef File : CDB->getAllFiles()) {
+  auto PI = getProjectInfo(File);

here we're locking/unlocking the mutex maybe thousands of times, to do mostly 
redundant lookups into the cache

Seems worth doing at least one of:
 - deduplicate directories, so we lock once for each distinct directory and 
build a string->bool map. Then use that map to filter the results
 - lock around the whole loop and use getCDBInDirLocked()



Comment at: clang-tools-extra/clangd/index/Background.cpp:653
+auto PI = CDB.getProjectInfo(File);
+assert(PI && "Found CDB but no ProjectInfo!");
+

This looks like a bad assertion: it should be OK to provide compile commands 
but not project info.

(Otherwise getProjectInfo should be pure virtual, but I'm concerned about the 
raciness if we're going to insist they're consistent but not provide any way of 
synchronizing)

I'd suggest we should just not index such files (for now). Later we can start 
passing "" to the index storage factory to get the fallback storage (I think 
today we pass "", but the factory doesn't handle it - oops!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64247



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

[PATCH] D64247: [clangd] Filter out non-governed files from broadcast

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

This also turns off implicit discovery of additional compilation
databases.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64247

Files:
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/GlobalCompilationDatabase.h
  clang-tools-extra/clangd/QueryDriverDatabase.cpp
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
  clang-tools-extra/clangd/unittests/TestFS.cpp
  clang-tools-extra/clangd/unittests/TestFS.h

Index: clang-tools-extra/clangd/unittests/TestFS.h
===
--- clang-tools-extra/clangd/unittests/TestFS.h
+++ clang-tools-extra/clangd/unittests/TestFS.h
@@ -12,6 +12,8 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
 #include "ClangdServer.h"
+#include "GlobalCompilationDatabase.h"
+#include "Path.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
@@ -48,7 +50,9 @@
   StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
-  getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override;
+  getCompileCommand(PathRef File) const override;
+
+  llvm::Optional getProjectInfo(PathRef File) const override;
 
   std::vector ExtraClangFlags;
 
Index: clang-tools-extra/clangd/unittests/TestFS.cpp
===
--- clang-tools-extra/clangd/unittests/TestFS.cpp
+++ clang-tools-extra/clangd/unittests/TestFS.cpp
@@ -6,7 +6,11 @@
 //
 //===--===//
 #include "TestFS.h"
+#include "GlobalCompilationDatabase.h"
+#include "Path.h"
 #include "URI.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
@@ -36,9 +40,13 @@
   // -ffreestanding avoids implicit stdc-predef.h.
 }
 
+llvm::Optional
+MockCompilationDatabase::getProjectInfo(PathRef File) const {
+  return ProjectInfo{Directory};
+};
+
 llvm::Optional
-MockCompilationDatabase::getCompileCommand(PathRef File,
-   ProjectInfo *Project) const {
+MockCompilationDatabase::getCompileCommand(PathRef File) const {
   if (ExtraClangFlags.empty())
 return None;
 
@@ -57,8 +65,6 @@
 CommandLine.push_back(RelativeFilePath.str());
   }
 
-  if (Project)
-Project->SourceRoot = Directory;
   return {tooling::CompileCommand(Directory != llvm::StringRef()
   ? Directory
   : llvm::sys::path::parent_path(File),
Index: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
===
--- clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
+++ clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
@@ -8,10 +8,21 @@
 
 #include "GlobalCompilationDatabase.h"
 
+#include "Path.h"
 #include "TestFS.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -20,8 +31,10 @@
 using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::EndsWith;
+using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::StartsWith;
+using ::testing::UnorderedElementsAre;
 
 TEST(GlobalCompilationDatabaseTest, FallbackCommand) {
   DirectoryBasedGlobalCompilationDatabase DB(None);
@@ -50,13 +63,9 @@
   class BaseCDB : public GlobalCompilationDatabase {
   public:
 llvm::Optional
-getCompileCommand(llvm::StringRef File,
-  ProjectInfo *Project) const override {
-  if (File == testPath("foo.cc")) {
-if (Project)
-  Project->SourceRoot = testRoot();
+getCompileCommand(llvm::StringRef File) const override {
+  if (File == testPath("foo.cc"))
 return cmd(File, "-DA=1");
-  }
   return None;
 }
 
@@ -64,6 +73,10 @@
 getFallbackCommand(llvm::StringRef File) const override {
   return cmd(File, "-DA=2");
 }
+
+llvm::Optional getProjectInfo(PathRef File) const override {
+  return ProjectInfo{testRoot()};
+}
   };
 
 protected:
@@ -153,6 +166,109 @@