[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D48903#1189056, @ilya-biryukov wrote:

> I see, thanks for the explanation.
>
> LGTM for the update revision, given we have confirmation the tests pass on 
> Windows.


Thanks, I'll push it, let's hope this time is the right time!


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC339063: [VirtualFileSystem] InMemoryFileSystem::status: 
Return a Status with the… (authored by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48903?vs=158625&id=159401#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  test/PCH/case-insensitive-include.c
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: test/PCH/case-insensitive-include.c
===
--- test/PCH/case-insensitive-include.c
+++ test/PCH/case-insensitive-include.c
@@ -13,7 +13,7 @@
 // RUN: touch -r %t-dir/case-insensitive-include.h %t.copy
 // RUN: mv %t.copy %t-dir/case-insensitive-include.h
 
-// RUN: %clang_cc1 -fsyntax-only %s -include-pch %t.pch -I %t-dir -verify
+// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include-pch %t.pch -I %t-dir -verify
 
 // expected-no-diagnostics
 
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -315,9 +315,11 @@
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
-  if (UFE.File)
-if (auto RealPathName = UFE.File->getName())
-  UFE.RealPathName = *RealPathName;
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE.RealPathName = RealPathName.str();
+
   return &UFE;
 }
 
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -474,12 +474,28 @@
   Status Stat;
   InMemoryNodeKind Kind;
 
+protected:
+  /// Return Stat.  This should only be used for internal/debugging use.  When
+  /// clients wants the Status of this node, they should use
+  /// \p getStatus(StringRef).
+  const Status &getStatus() const { return Stat; }
+
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
   : Stat(std::move(Stat)), Kind(Kind) {}
   virtual ~InMemoryNode() = default;
 
-  const Status &getStatus() const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+
+  /// Get the filename of this node (the name without the directory part).
+  StringRef getFileName() const {
+return llvm::sys::path::filename(Stat.getName());
+  }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -504,14 +520,21 @@
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile &Node;
+  /// The name to use when returning a Status for this file.
+  std::string RequestedName;
 
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile &Node) : Node(Node) {}
+  explicit InMemoryFileAdaptor(InMemoryFile &Node, std::string RequestedName)
+  : Node(Node), RequestedName(std::move(RequestedName)) {}
 
-  llvm::ErrorOr status() override { return Node.getStatus(); }
+  llvm::ErrorOr status() override {
+return Node.getStatus(RequestedName);
+  }
 
   llvm::ErrorOr>
   getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +734,7 @@
 llvm::ErrorOr InMemoryFileSystem::status(const Twine &Path) {
   auto Node = lookupInMemoryNode(*this, Root.get(), Path);
   if (Node)
-return (*Node)->getStatus();
+return (*Node)->getStatus(Path.str());
   return Node.getError();
 }
 
@@ -724,7 +747,8 @@
   // When we have a file provide a heap-allocated wrapper for the memory buffer
   // to match the ownership semantics for File.
   if (auto *F = dyn_cast(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(
+new detail::InMemoryFileAdaptor(*F, Path.str()));
 
   // FIXME: errc::not_a_file?
   return make_error_code(llvm::errc::invalid_argument);
@@ -736,21 +760,33 @@
 class InMemoryDirIterator : public clang::vfs::detail::DirIterImpl {
   detail::InMemoryDirectory::const_iterator I;
   detail::InMemoryDirectory::const_iterator E;
+  std::string RequestedDirName;
+
+  void setCurrentEntry() {
+if (I != E) {
+  SmallString<256> Path(RequestedDirName);
+  llvm::sys::path::append(Path, I->second->getFileName());
+  CurrentEntry = I->second->getStatus(Path);
+} else {
+  // When we're at the end, make CurrentEntry inv

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 159764.
simark added a comment.
Herald added a subscriber: arphaman.

New version.  I tried to share the code between this and SymbolCollector, but
didn't find a good clean way.  Do you have concrete suggestions on how to do
this?  Otherwise, would it be acceptable to merge it as-is?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/FindSymbols.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.h
===
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -40,15 +40,22 @@
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not null, use that as the Directory field of the
+  /// CompileCommand.
+  ///
+  /// If \p RelPathPrefix is not null, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.
+  MockCompilationDatabase(StringRef Directory = StringRef(),
+  StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
   getCompileCommand(PathRef File) const override;
 
   std::vector ExtraClangFlags;
-  const bool UseRelPaths;
+
+private:
+  StringRef Directory;
+  StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -32,22 +32,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: ExtraClangFlags({"-ffree

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 159768.
simark added a comment.

Formatting.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/FindSymbols.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.h
===
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -40,15 +40,22 @@
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not null, use that as the Directory field of the
+  /// CompileCommand.
+  ///
+  /// If \p RelPathPrefix is not null, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.
+  MockCompilationDatabase(StringRef Directory = StringRef(),
+  StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
   getCompileCommand(PathRef File) const override;
 
   std::vector ExtraClangFlags;
-  const bool UseRelPaths;
+
+private:
+  StringRef Directory;
+  StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -32,22 +32,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+MockCompilationDatabase::MockCompilationDatabase(StringRef Directory,
+ StringRef RelPathPrefix)
+: ExtraClangFlags({"-ffreestanding"}), Directory(

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark marked 3 inline comments as done.
simark added a comment.

In https://reviews.llvm.org/D48687#1193470, @hokein wrote:

> Sorry for the delay. I thought there was a dependent patch of this patch, but 
> it seems resolved, right?
>
> A rough round of review.


Right, the patch this one depends on is in clang, and there does not seem to be 
regressions this time.

>> New version. I tried to share the code between this and SymbolCollector, but
>>  didn't find a good clean way. Do you have concrete suggestions on how to do
>>  this? Otherwise, would it be acceptable to merge it as-is?
> 
> Yeah, I'm fine with the current change. I was not aware of the 
> `getAbsoluteFilePath` has been pulled out to the `SourceCode.h`. I think the 
> SymbolCollector could use this function as well (but that's out of scope of 
> this patch).

Thanks.




Comment at: clangd/SourceCode.h:65
 /// Get the absolute file path of a given file entry.
-llvm::Optional getAbsoluteFilePath(const FileEntry *F,
-const SourceManager 
&SourceMgr);
+llvm::Optional getRealPath(const FileEntry *F,
+const SourceManager &SourceMgr);

hokein wrote:
> Why rename this function?
> 
>  Is it guaranteed that `RealPath` is always an absolute path?
Before, it only made sure that the path was absolute, now it goes a step 
further and makes it a "real path", resolving symlinks and removing `.` and 
`..`.  When we talk about a "real path", it refers to the unix realpath syscall:

http://man7.org/linux/man-pages/man3/realpath.3.html

So yes, the result is guaranteed to be absolute too.



Comment at: unittests/clangd/TestFS.cpp:52
   CommandLine.insert(CommandLine.begin(), "clang");
-  CommandLine.insert(CommandLine.end(), UseRelPaths ? FileName : File);
-  return {tooling::CompileCommand(sys::path::parent_path(File), FileName,
-  std::move(CommandLine), "")};
+  if (RelPathPrefix == StringRef()) {
+// Use the absolute path in the compile command.

hokein wrote:
> Can we use `RelPathPrefix.empty()` instead of comparing with `StringRef()`?
Done.



Comment at: unittests/clangd/XRefsTests.cpp:325
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];

hokein wrote:
> It seems that there is no difference between `HeaderInPreambleAnnotations` 
> and `HeaderNotInPreambleAnnotations` in the test. Both of them are treated 
> equally.
> 
The difference is that one is included in the main file as part of the preamble 
and the other is out of the preamble.  Since they take different code paths in 
clang, I think it's good to test both.  At some point, I had a similar bug that 
would only happen with a header included in the preamble.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687



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

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 159972.
simark marked an inline comment as done.
simark added a comment.

Replace

  RelPathPrefix == StringRef()

with

  RelPathPrefix.empty()


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/FindSymbols.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.h
===
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -40,15 +40,22 @@
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not null, use that as the Directory field of the
+  /// CompileCommand.
+  ///
+  /// If \p RelPathPrefix is not null, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.
+  MockCompilationDatabase(StringRef Directory = StringRef(),
+  StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
   getCompileCommand(PathRef File) const override;
 
   std::vector ExtraClangFlags;
-  const bool UseRelPaths;
+
+private:
+  StringRef Directory;
+  StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -32,22 +32,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+MockCompilationDatabase::MockCompilationDatabase(StringRef Directory,
+

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark marked 8 inline comments as done.
simark added inline comments.



Comment at: clangd/SourceCode.h:69
 
+llvm::Optional getRealPath(const FileEntry *F,
+const SourceManager &SourceMgr);

ilya-biryukov wrote:
> This function looks like a good default choice for normalizing paths before 
> putting them into LSP structs, ClangdServer responses, etc.
> I suggest we add a small comment here with a guideline for everyone to 
> attempt using it whenever possible. WDYT?
What about this:

```
/// Get the real/canonical path of \p F.  This means:
///
///   - Absolute path
///   - Symlinks resolved
///   - No "." or ".." component
///   - No duplicate or trailing directory separator
///
/// This function should be used when sending paths to clients, so that paths
/// are normalized as much as possible.
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687



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

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark marked an inline comment as done.
simark added a comment.

In https://reviews.llvm.org/D48687#1195840, @hokein wrote:

> Looks good with a few nits. Looks like you didn't update the patch correctly. 
> You have marked comments done, but your code doesn't get changed accordingly. 
> Please double check with it.
>
> I tried your patch and it did fix the duplicated issue I encountered before. 
> Thanks for fixing it!


Ah sorry, I applied the fixes locally, but was waiting for Ilya's feedback for 
the function comment.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687



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

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 160175.
simark marked 4 inline comments as done.
simark added a comment.

Latest update.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/FindSymbols.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.h
===
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -40,15 +40,22 @@
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not empty, use that as the Directory field of the
+  /// CompileCommand.
+  ///
+  /// If \p RelPathPrefix is not empty, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.
+  MockCompilationDatabase(StringRef Directory = StringRef(),
+  StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
   getCompileCommand(PathRef File) const override;
 
   std::vector ExtraClangFlags;
-  const bool UseRelPaths;
+
+private:
+  StringRef Directory;
+  StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -32,22 +32,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+MockCompilationDatabase::MockCompilationDatabase(StringRef Directory,
+ StringRef RelPathPrefix)
+: E

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE339483: [clangd] Avoid duplicates in findDefinitions 
response (authored by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48687?vs=160175&id=160197#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/FindSymbols.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -32,22 +32,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+MockCompilationDatabase::MockCompilationDatabase(StringRef Directory,
+ StringRef RelPathPrefix)
+: ExtraClangFlags({"-ffreestanding"}), Directory(Directory),
+  RelPathPrefix(RelPathPrefix) {
   // -ffreestanding avoids implicit stdc-predef.h.
 }
 
 Optional
 MockCompilationDatabase::getCompileCommand(PathRef File) const {
   if (ExtraClangFlags.empty())
 return None;
 
-  auto CommandLine = ExtraClangFlags;
   auto FileName = sys::path::filename(File);
+
+  // Build the compile command.
+  auto CommandLine = ExtraClangFlags;
   CommandLine.insert(CommandLine.begin(), "clang");
-  CommandLine.insert(CommandLine.end(), UseRelPaths ? FileName : File);
-  return {tooling::CompileCommand(sys::path::parent_path(File), FileName,
-  std::move(CommandLine), "")};
+  if (RelPathPrefix.empty()) {
+// Use the absolute path in the compile command.
+CommandLine.push_back(File);
+  } else {
+// Build a relative path using RelPathPrefix.
+SmallString<32> RelativeFilePath(RelPathPrefix);
+llvm::sys::path::append(RelativeFilePath, FileName)

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/SourceCode.h:65
 
 /// Get the absolute file path of a given file entry.
 TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,

hokein wrote:
> nit: this comment is not needed.
Woops, merge mistake.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687



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

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: lib/Basic/FileManager.cpp:319
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
+  if (UFE.File) {
+if (auto Path = UFE.File->getName()) {

What's the rationale for only computing the field if `UFE.File` is non-null?

Previously, if you looked up the file with `openFile == false` and then later 
`openFile == true`, the `RealPathName` field would not be set because of this.  
That doesn't seem right.



Comment at: lib/Basic/FileManager.cpp:326
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
+}

If the path contains symlinks, doesn't this put a non-real path in the 
RealPathName field?  Won't users (e.g. clangd) use this value thinking it is a 
real path, when it is actually not?


Repository:
  rC Clang

https://reviews.llvm.org/D51159



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

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: lib/Basic/FileManager.cpp:319
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
+  if (UFE.File) {
+if (auto Path = UFE.File->getName()) {

ioeric wrote:
> simark wrote:
> > What's the rationale for only computing the field if `UFE.File` is non-null?
> > 
> > Previously, if you looked up the file with `openFile == false` and then 
> > later `openFile == true`, the `RealPathName` field would not be set because 
> > of this.  That doesn't seem right.
> There has been no guarantee that RealFilePath is always set. I think that's 
> the reason why the acceasor is called tryGetRealPathName.
The way I understood it was that it could be empty because computing the real 
path can fail.  Not just because we didn't skipped computing it.



Comment at: lib/Basic/FileManager.cpp:326
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
+}

ioeric wrote:
> simark wrote:
> > If the path contains symlinks, doesn't this put a non-real path in the 
> > RealPathName field?  Won't users (e.g. clangd) use this value thinking it 
> > is a real path, when it is actually not?
> This was the original behavior. In general, File Manager should never call 
> real_path for users because it can be very expensive. Users should call 
> real_path if they want to resolve symlinks. That said, it's fair to say that 
> "RealPathName" is just a wrong name, and we should clean it up at some point.
Ok, then if the goal is not to actually have a real path (in the realpath(3) 
sense), that's fine.  But I think we should rename the field sooner than later, 
it's really confusing.

That also means that it's kind of useless for us in clangd, so we should always 
call real_path there and not rely on that field.


Repository:
  rC Clang

https://reviews.llvm.org/D51159



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

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: lib/Basic/FileManager.cpp:319
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
+  if (UFE.File) {
+if (auto Path = UFE.File->getName()) {

ioeric wrote:
> simark wrote:
> > ioeric wrote:
> > > simark wrote:
> > > > What's the rationale for only computing the field if `UFE.File` is 
> > > > non-null?
> > > > 
> > > > Previously, if you looked up the file with `openFile == false` and then 
> > > > later `openFile == true`, the `RealPathName` field would not be set 
> > > > because of this.  That doesn't seem right.
> > > There has been no guarantee that RealFilePath is always set. I think 
> > > that's the reason why the acceasor is called tryGetRealPathName.
> > The way I understood it was that it could be empty because computing the 
> > real path can fail.  Not just because we didn't skipped computing it.
> I agree that the API is confusing and lack of documentation (and we should 
> fix), but the previous implementation did skip the computation if File is not 
> available though. 
> I agree that the API is confusing and lack of documentation (and we should 
> fix), but the previous implementation did skip the computation if File is not 
> available though.

Did it have a reason not to?  What is the `RealPathName` field useful for, if 
it's unreliable?



Comment at: lib/Basic/FileManager.cpp:326
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
+}

ioeric wrote:
> simark wrote:
> > ioeric wrote:
> > > simark wrote:
> > > > If the path contains symlinks, doesn't this put a non-real path in the 
> > > > RealPathName field?  Won't users (e.g. clangd) use this value thinking 
> > > > it is a real path, when it is actually not?
> > > This was the original behavior. In general, File Manager should never 
> > > call real_path for users because it can be very expensive. Users should 
> > > call real_path if they want to resolve symlinks. That said, it's fair to 
> > > say that "RealPathName" is just a wrong name, and we should clean it up 
> > > at some point.
> > Ok, then if the goal is not to actually have a real path (in the 
> > realpath(3) sense), that's fine.  But I think we should rename the field 
> > sooner than later, it's really confusing.
> > 
> > That also means that it's kind of useless for us in clangd, so we should 
> > always call real_path there and not rely on that field.
> > Ok, then if the goal is not to actually have a real path (in the 
> > realpath(3) sense), that's fine. But I think we should rename the field 
> > sooner than later, it's really confusing.
> +1
> 
> > That also means that it's kind of useless for us in clangd, so we should 
> > always call real_path there and not rely on that field.
> I guess it depends on whether you want symlink resolution or not. I know that 
> clangd's index symbol collector resolves symlink explicitly, but I don't 
> think clangd enforces symlink resolution in general?
> I guess it depends on whether you want symlink resolution or not. I know that 
> clangd's index symbol collector resolves symlink explicitly, but I don't 
> think clangd enforces symlink resolution in general?

If we don't, we probably risk having duplicate results similar to what

  https://reviews.llvm.org/D48687

fixed, by with paths differing because of symlinks instead of dot-dots.


Repository:
  rC Clang

https://reviews.llvm.org/D51159



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

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: lib/Basic/FileManager.cpp:319
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
+  if (UFE.File) {
+if (auto Path = UFE.File->getName()) {

ioeric wrote:
> simark wrote:
> > ioeric wrote:
> > > simark wrote:
> > > > ioeric wrote:
> > > > > simark wrote:
> > > > > > What's the rationale for only computing the field if `UFE.File` is 
> > > > > > non-null?
> > > > > > 
> > > > > > Previously, if you looked up the file with `openFile == false` and 
> > > > > > then later `openFile == true`, the `RealPathName` field would not 
> > > > > > be set because of this.  That doesn't seem right.
> > > > > There has been no guarantee that RealFilePath is always set. I think 
> > > > > that's the reason why the acceasor is called tryGetRealPathName.
> > > > The way I understood it was that it could be empty because computing 
> > > > the real path can fail.  Not just because we didn't skipped computing 
> > > > it.
> > > I agree that the API is confusing and lack of documentation (and we 
> > > should fix), but the previous implementation did skip the computation if 
> > > File is not available though. 
> > > I agree that the API is confusing and lack of documentation (and we 
> > > should fix), but the previous implementation did skip the computation if 
> > > File is not available though.
> > 
> > Did it have a reason not to?  What is the `RealPathName` field useful for, 
> > if it's unreliable?
> I think the biggest concern is the performance.
>  
> For example, clangd code completion latency increased dramatically with 
> `real_path`:
> With `real_path`:
> {F7039629}
> {F7041680}
> 
> Wihtou `real_path`:
> {F7039630}
But with this patch, we are not using real_path anymore, so it can't be the 
reason for not computing `RealPathName` when not opening the file.  Since the 
non-real_path method is much more lightweight, would it make a significant 
difference if we always computed the field?

In the end I don't really mind, I am just looking for a rational explanation to 
why it's done this way.



Comment at: lib/Basic/FileManager.cpp:326
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
+}

ioeric wrote:
> simark wrote:
> > ioeric wrote:
> > > simark wrote:
> > > > ioeric wrote:
> > > > > simark wrote:
> > > > > > If the path contains symlinks, doesn't this put a non-real path in 
> > > > > > the RealPathName field?  Won't users (e.g. clangd) use this value 
> > > > > > thinking it is a real path, when it is actually not?
> > > > > This was the original behavior. In general, File Manager should never 
> > > > > call real_path for users because it can be very expensive. Users 
> > > > > should call real_path if they want to resolve symlinks. That said, 
> > > > > it's fair to say that "RealPathName" is just a wrong name, and we 
> > > > > should clean it up at some point.
> > > > Ok, then if the goal is not to actually have a real path (in the 
> > > > realpath(3) sense), that's fine.  But I think we should rename the 
> > > > field sooner than later, it's really confusing.
> > > > 
> > > > That also means that it's kind of useless for us in clangd, so we 
> > > > should always call real_path there and not rely on that field.
> > > > Ok, then if the goal is not to actually have a real path (in the 
> > > > realpath(3) sense), that's fine. But I think we should rename the field 
> > > > sooner than later, it's really confusing.
> > > +1
> > > 
> > > > That also means that it's kind of useless for us in clangd, so we 
> > > > should always call real_path there and not rely on that field.
> > > I guess it depends on whether you want symlink resolution or not. I know 
> > > that clangd's index symbol collector resolves symlink explicitly, but I 
> > > don't think clangd enforces symlink resolution in general?
> > > I guess it depends on whether you want symlink resolution or not. I know 
> > > that clangd's index symbol collector resolves symlink explicitly, but I 
> > > don't think clangd enforces symlink resolution in general?
> > 
> > If we don't, we probably risk having duplicate results similar to what
> > 
> >   https://reviews.llvm.org/D48687
> > 
> > fixed, by with paths differing because of symlinks instead of dot-dots.
> Was the bug addressed in D48687 actually caused by symlinks though? If want 
> we want is absolute paths with dotdot cleaned, it should be much cheaper to 
> call `VFS::makeAbsolutePath` with `remove_dot_dot`.
> 
> In general, it's unclear whether clangd should resolve symlink (it might not 
> always be what users want), and it should probably be a decision made by the 
> build system integration. I think we would need a much more careful design if 
> we decide to handle symlinks in clangd. 
> Was the bug addressed in D48687 actually cau

[PATCH] D51311: (WIP) Type hierarchy

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, ioeric, 
ilya-biryukov.

This is a work in progress patch to support an eventual "get type
hierarchy" request that does not exist in the LSP today.  I only plan to
support getting parent types (i.e. base classes) for now, because it
doesn't require touching the index.  Finding child classes would come
later.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51311

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -27,6 +27,7 @@
 
 namespace {
 using testing::ElementsAre;
+using testing::Eq;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
@@ -1068,6 +1069,166 @@
   ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
 }
 
+TEST(TypeHierarchy, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+$ParentDef^struct Parent
+{
+  int a;
+};
+
+$Child1Def^struct Child1 : Parent
+{
+  int b;
+};
+
+struct Ch$p1^ild2 : Child1
+{
+  int c;
+};
+
+struct Child3 : Child2
+{
+  int d;
+};
+
+int main()
+{
+  Ch$p2^ild2 ch$p3^ild2;
+
+  parent.a = 1;
+  ch$p4^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  llvm::Optional Result;
+
+  TypeHierarchy ExpectedResult{{TypeHierarchyResult{
+  "Child1",
+  Source.point("Child1Def"),
+  false,
+  {TypeHierarchyResult{"Parent", Source.point("ParentDef"), false, {}};
+
+  for (auto Pt : {"p1", "p2", "p3", "p4"}) {
+Result = getTypeHierarchy(AST, Source.point(Pt));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+  }
+}
+
+TEST(TypeHierarchy, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+$Parent1Def^struct Parent1
+{
+  int a;
+};
+
+$Parent2Def^struct Parent2
+{
+  int b;
+};
+
+$Parent3Def^struct Parent3 : Parent2
+{
+  int c;
+};
+
+struct Ch$c1^ild : Parent1, Parent3
+{
+  int d;
+};
+
+int main()
+{
+  Ch$c2^ild  ch$c3^ild;
+
+  ch$c4^ild.a = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  llvm::Optional Result;
+  TypeHierarchy ExpectedResult{{
+  TypeHierarchyResult{"Parent1", Source.point("Parent1Def"), false, {}},
+  TypeHierarchyResult{
+  "Parent3",
+  Source.point("Parent3Def"),
+  false,
+  {TypeHierarchyResult{
+  "Parent2", Source.point("Parent2Def"), false, {,
+  }};
+
+  for (auto Pt : {"c1", "c2", "c3", "c4"}) {
+Result = getTypeHierarchy(AST, Source.point(Pt));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+  }
+}
+
+TEST(TypeHierarchy, OnMethod) {
+  Annotations Source(R"cpp(
+$ParentDef^struct Parent
+{
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+$Child1Def^struct Child1 : Parent
+{
+  void method ();
+  void method (char x);
+};
+
+struct Child2 : Child1
+{
+  void met$p1^hod ();
+  void met$p2^hod (int x);
+};
+
+struct Child3 : Child2
+{
+  void method (int x);
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  {
+TypeHierarchy ExpectedResult{{TypeHierarchyResult{
+"Child1",
+Source.point("Child1Def"),
+true,
+{TypeHierarchyResult{"Parent", Source.point("ParentDef"), true, {}};
+
+llvm::Optional Result =
+getTypeHierarchy(AST, Source.point("p1"));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+  }
+
+  {
+TypeHierarchy ExpectedResult{{TypeHierarchyResult{
+"Child1",
+Source.point("Child1Def"),
+false,
+{TypeHierarchyResult{"Parent", Source.point("ParentDef"), true, {}};
+
+llvm::Optional Result =
+getTypeHierarchy(AST, Source.point("p2"));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/XRefs.h
===
--- clangd/XRefs.h
+++ clangd/XRefs.h
@@ -34,6 +34,9 @@
 /// Get the hover information when hovering at \p Pos.
 llvm::Optional getHover(ParsedAST &AST, Position Pos);
 
+/// Get the type hierarchy information at \p Pos.
+llvm::Optional getTypeHierarchy(ParsedAST &AST, Position Pos);
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -

[PATCH] D51311: (WIP) Type hierarchy

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/Protocol.h:889
+  // Does this node implement the method targeted by the request?
+  bool DeclaresMethod;
+

kadircet wrote:
> I think comment and the name is in contradiction here, do you mean 
> DefinesMethod?
Actually I think the comment is wrong.  Even if we only see a declaration of 
the method, it's enough to say that this type has its own version of the method.



Comment at: clangd/XRefs.cpp:669
+  const CXXMethodDecl *Candidate) {
+  // FIXME: How do I determine if Method overrides Candidate?
+

kadircet wrote:
> https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp
> 
> you can check this sample, which simply traverses all base classes and gets 
> methods with the same name, then checks whether one is overload of the other. 
> If it they are not overloads then one in the base classes gets overriden by 
> the other.
> 
> 
> Another approach could be to search for one method in others 
> overriden_methods.
> 
> But they are both inefficient, I would suggest calling these functions only 
> when one of them is defined in the base class of other then you can just 
> check for name equality and not being an overload.
> https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp

Did you mean to link to a particular line?

> you can check this sample, which simply traverses all base classes and gets 
> methods with the same name, then checks whether one is overload of the other. 
> If it they are not overloads then one in the base classes gets overriden by 
> the other.

> Another approach could be to search for one method in others 
> overriden_methods.

I have tried using `overriden_methods`, but it only contains methods marked 
virtual.  For this feature, I would like to consider non-virtual methods too.

> But they are both inefficient, I would suggest calling these functions only 
> when one of them is defined in the base class of other then you can just 
> check for name equality and not being an overload.

I am not sure I understand, but maybe it will be clearer when I will see the 
sample.



Comment at: clangd/XRefs.cpp:693
+
+std::cerr << " >>> A parent is " << ParentDecl->getName().str()
+  << std::endl;

kadircet wrote:
> If you plan to keep it for later debugging info, consider using {d,v}log
Yes of course :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51311



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

[PATCH] D51311: (WIP) Type hierarchy

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/Protocol.h:891
+
+  std::vector Parents;
+

ilya-biryukov wrote:
> What is the proposal to add children (i.e. overriden methods) to the 
> hierarchy?
> This seems like a place where we might want to have multiple requests from 
> the clients when expanding the nodes.
In my prototype, I fetch the whole parent hierarchy in a single request.  In 
the proposal at

  https://github.com/Microsoft/vscode-languageserver-node/pull/346

it has been suggested to only fetch one level at the time, and have the client 
issue as many request as it wants (until possibly getting the whole hierarchy). 
 I don't know what the protocol will end up like. 



Comment at: clangd/ProtocolHandlers.cpp:70
   Register("textDocument/hover", &ProtocolCallbacks::onHover);
+  Register("textDocument/typeHierarchy", &ProtocolCallbacks::onTypeHierarchy);
   Register("textDocument/documentSymbol", 
&ProtocolCallbacks::onDocumentSymbol);

kadircet wrote:
> LSP doesn't have such an entry, maybe we should use [[ 
> https://microsoft.github.io/language-server-protocol/specification#workspace_executeCommand
>  | workspace/executeCommand ]]. WDYT @ilya-biryukov 
I don't intend to merge this patch before the protocol actually defines the way 
to get type hierarchy.  So this will be updated to reflect what the protocol 
will actually define.



Comment at: clangd/XRefs.cpp:669
+  const CXXMethodDecl *Candidate) {
+  // FIXME: How do I determine if Method overrides Candidate?
+

ilya-biryukov wrote:
> kadircet wrote:
> > ilya-biryukov wrote:
> > > simark wrote:
> > > > kadircet wrote:
> > > > > https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp
> > > > > 
> > > > > you can check this sample, which simply traverses all base classes 
> > > > > and gets methods with the same name, then checks whether one is 
> > > > > overload of the other. If it they are not overloads then one in the 
> > > > > base classes gets overriden by the other.
> > > > > 
> > > > > 
> > > > > Another approach could be to search for one method in others 
> > > > > overriden_methods.
> > > > > 
> > > > > But they are both inefficient, I would suggest calling these 
> > > > > functions only when one of them is defined in the base class of other 
> > > > > then you can just check for name equality and not being an overload.
> > > > > https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp
> > > > 
> > > > Did you mean to link to a particular line?
> > > > 
> > > > > you can check this sample, which simply traverses all base classes 
> > > > > and gets methods with the same name, then checks whether one is 
> > > > > overload of the other. If it they are not overloads then one in the 
> > > > > base classes gets overriden by the other.
> > > > 
> > > > > Another approach could be to search for one method in others 
> > > > > overriden_methods.
> > > > 
> > > > I have tried using `overriden_methods`, but it only contains methods 
> > > > marked virtual.  For this feature, I would like to consider non-virtual 
> > > > methods too.
> > > > 
> > > > > But they are both inefficient, I would suggest calling these 
> > > > > functions only when one of them is defined in the base class of other 
> > > > > then you can just check for name equality and not being an overload.
> > > > 
> > > > I am not sure I understand, but maybe it will be clearer when I will 
> > > > see the sample.
> > > See the code that actually computes a list for `override_methods()`: 
> > > Sema::AddOverriddenMethods.
> > > Did you mean to link to a particular line?
> > 
> > 
> > Yeah sorry, I was trying to link the function ilya mentioned.
> > https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L7615
> > 
> > Since you also mentioned checking non-virtual method as well you might 
> > wanna look at 
> > https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L7555
> >  as well.
> > 
> > Also I got the chance to look the rest of the patch, as I mentioned above 
> > you are already checking two methods iff one lies in the others base 
> > classes. So you can simply check for name equality and not being an 
> > overload case.
> Also wanted to bring up what @sammccall already mentioned on clangd-dev: we 
> probably shouldn't show methods that hide base methods rather than override 
> the virtual base methods (at least, by default).
> 
> Those might be useful, but unless we can have a clear UI indicator of whether 
> a method is overriding a virtual base method or is hiding some other method, 
> it would to add more confusion than benefit IMO.
> Also I got the chance to look the rest of the patch, as I mentioned above you 
> are already checking two methods iff one lies in the others base classes. So 
> you can simply check for name equality and not being an overload case.

To check for

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, ioeric, 
ilya-biryukov.

It is currently possible to tell clangd where to find the
compile_commands.json file through the initializationOptions or the
didChangeConfiguration message.  However, it is not possible to tell
clangd to deselect any explicit compilation database path (i.e. go back
to the default).

This patch makes it possible by sending the value null:

  params: {
"settings": {
  "compilationDatabasePath": null
}
  }

Not including the compilationDatabasePath field doesn't change the value
(which means it doesn't deselect it if one is already set).  I chose to
do it this way because the other possible field,
compilationDatabaseChanges, just contains a delta.  So I think it makes
sense if compilationDatabasePath works the same way.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/compile-commands-path.test

Index: test/clangd/compile-commands-path.test
===
--- test/clangd/compile-commands-path.test
+++ test/clangd/compile-commands-path.test
@@ -39,4 +39,11 @@
 # CHECK-NEXT:   {
 # CHECK-NEXT: "message": "MACRO is two",
 ---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":null}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is not defined",
+---
 {"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -358,7 +358,14 @@
 /// "initialize" request and for the "workspace/didChangeConfiguration"
 /// notification since the data received is described as 'any' type in LSP.
 struct ClangdConfigurationParamsChange {
-  llvm::Optional compilationDatabasePath;
+  /// Path to the directory containing the compile_commands.json file to use.
+  /// The outer Optional is empty if the compilationDatabasePath field was not
+  /// present at all in the request.
+  /// The inner Optional is empty if the compilationDatabasePath field was
+  /// preset, but the value was null.
+  /// If the value of the compilationDatabasePath in the request is a string,
+  /// both Optionals are instantiated.
+  llvm::Optional> compilationDatabasePath;
 
   // The changes that happened to the compilation database.
   // The key of the map is a file name.
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -609,11 +609,22 @@
  O.map("compilationCommand", CDbUpdate.compilationCommand);
 }
 
-bool fromJSON(const json::Value &Params,
+bool fromJSON(const json::Value &Settings,
   ClangdConfigurationParamsChange &CCPC) {
-  json::ObjectMapper O(Params);
-  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath) &&
- O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
+  json::ObjectMapper O(Settings);
+  const json::Value *P = Settings.getAsObject()->get("compilationDatabasePath");
+
+  if (P) {
+// The field is present.
+CCPC.compilationDatabasePath.emplace();
+llvm::Optional S = P->getAsString();
+if (S) {
+  // The field has a string value.
+  CCPC.compilationDatabasePath->emplace(*S);
+}
+  }
+
+  return O && O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
 }
 
 bool fromJSON(const json::Value &Params, ReferenceParams &R) {
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -63,7 +63,7 @@
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
   /// Set the compile commands directory to \p P.
-  void setCompileCommandsDir(Path P);
+  void setCompileCommandsDir(llvm::Optional P);
 
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -60,7 +60,7 @@
   return C;
 }
 
-void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(Path P) {
+void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(llvm::Optional P) {
   std::lock_guard Lock(Mutex);
   CompileCommandsDir = P;
   CompilationDatabases.clear();
Index: clangd/ClangdLSPServer.h
==

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 164183.
simark added a comment.

Fix formatting.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/compile-commands-path.test

Index: test/clangd/compile-commands-path.test
===
--- test/clangd/compile-commands-path.test
+++ test/clangd/compile-commands-path.test
@@ -39,4 +39,11 @@
 # CHECK-NEXT:   {
 # CHECK-NEXT: "message": "MACRO is two",
 ---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":null}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is not defined",
+---
 {"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -358,7 +358,14 @@
 /// "initialize" request and for the "workspace/didChangeConfiguration"
 /// notification since the data received is described as 'any' type in LSP.
 struct ClangdConfigurationParamsChange {
-  llvm::Optional compilationDatabasePath;
+  /// Path to the directory containing the compile_commands.json file to use.
+  /// The outer Optional is empty if the compilationDatabasePath field was not
+  /// present at all in the request.
+  /// The inner Optional is empty if the compilationDatabasePath field was
+  /// preset, but the value was null.
+  /// If the value of the compilationDatabasePath in the request is a string,
+  /// both Optionals are instantiated.
+  llvm::Optional> compilationDatabasePath;
 
   // The changes that happened to the compilation database.
   // The key of the map is a file name.
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -609,10 +609,22 @@
  O.map("compilationCommand", CDbUpdate.compilationCommand);
 }
 
-bool fromJSON(const json::Value &Params,
+bool fromJSON(const json::Value &Settings,
   ClangdConfigurationParamsChange &CCPC) {
-  json::ObjectMapper O(Params);
-  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath) &&
+  json::ObjectMapper O(Settings);
+  const json::Value *P = Settings.getAsObject()->get("compilationDatabasePath");
+
+  if (P) {
+// The field is present.
+CCPC.compilationDatabasePath.emplace();
+llvm::Optional S = P->getAsString();
+if (S) {
+  // The field has a string value.
+  CCPC.compilationDatabasePath->emplace(*S);
+}
+  }
+
+  return O &&
  O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
 }
 
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -63,7 +63,7 @@
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
   /// Set the compile commands directory to \p P.
-  void setCompileCommandsDir(Path P);
+  void setCompileCommandsDir(llvm::Optional P);
 
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -60,7 +60,8 @@
   return C;
 }
 
-void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(Path P) {
+void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(
+llvm::Optional P) {
   std::lock_guard Lock(Mutex);
   CompileCommandsDir = P;
   CompilationDatabases.clear();
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -126,9 +126,10 @@
 void setExtraFlagsForFile(PathRef File,
   std::vector ExtraFlags);
 
-/// Set the compile commands directory to \p P.
+/// Set the compile commands directory to \p P.  An empty Optional value
+/// means to not use an explicit compile commands directory path.
 /// Only valid for directory-based CDB, no-op and error log on InMemoryCDB;
-void setCompileCommandsDir(Path P);
+void setCompileCommandsDir(llvm::Optional P);
 
 /// Returns a CDB that should be used to get compile commands for the
 /// current instance of ClangdLSPServer.
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -613,14 

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Not sure who should review this, please feel free to add anybody who would be 
appropriate.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725



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

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D51725#1232092, @ilya-biryukov wrote:

> Wow, this is getting somewhat complicated.
>
> Have you considered rerunning clangd whenever someone changes an option like 
> that?
>  Would that be much more complicated on your side?
>
> Not opposed to having an option too, just want to be aware of the costs 
> involved on your end.


Since we already support changing the compilation database path at runtime, I 
don't think it's significantly more complex to support resetting that value to 
the default of empty/not-used.  If we can restart clangd when the users chooses 
to use no compilation database path, we might as well just restart it every 
time the user selects a new compilation database path.

I was assuming that restarting clangd might potentially be significantly more 
costly than just changing a setting, but maybe I'm wrong.  The point of 
switching compilation database path is usually to point to a different build 
that has different flags, so all open files will get reparsed anyway...  And 
just start clangd isn't particularly heavy.

I'll investigate how difficult it is to make our frontend (Theia 
) restart clangd when the user switches 
compilation database.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725



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

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D51725#1232748, @ilya-biryukov wrote:

> If this setting exposed directly the users in Theia or is it something that 
> is exposed via a custom UI (e.g. choosing named build configs or something 
> similar)?


It is through a custom UI, that we name exactly that, build configurations.  
There is an example of the corresponding configuration here:

https://github.com/theia-ide/theia/blob/master/packages/cpp/README.md

>> I'll investigate how difficult it is to make our frontend (Theia 
>> ) restart clangd when the user switches 
>> compilation database.
> 
> If that is feasible, might allow removing some code from clangd. Though don't 
> expect that amount to be too high.
>  One potential complication to restoring the state of the language server. It 
> should be just the list of open files, but I may be missing something.

I am not sure I understand correctly your last point.  Of course, when 
restarting clangd, we need to send again a `didOpen` for each file the user has 
currently open in the editor.  But that should be  it?

> If you'll decide to go with an option to reset the path, see the comment 
> about making empty path special. `Optional>` does not play nicely 
> with json serialization code and hard to read (even though it does look like 
> the right thing to do from the type system perspective).

Yes, noted.  I preferred to avoid giving a special meaning to an empty string 
because we could avoid it.  But I wouldn't mind changing it if we go that route.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725



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

[PATCH] D49253: [clangd] Fix category in clangd-vscode's package.json

[Simon Marchi via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337697: [clangd] Fix category in clangd-vscode's 
package.json (authored by simark, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D49253

Files:
  clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
@@ -9,7 +9,7 @@
 "vscode": "^1.18.0"
 },
 "categories": [
-"Languages",
+"Programming Languages",
 "Linters",
 "Snippets"
 ],


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
@@ -9,7 +9,7 @@
 "vscode": "^1.18.0"
 },
 "categories": [
-"Languages",
+"Programming Languages",
 "Linters",
 "Snippets"
 ],
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D49267#1171286, @ilya-biryukov wrote:

> Thanks for putting up this change! It can be really annoying that clangd does 
> not pick up the compile commands that got updated.
>
> A few things of the top of my head on why we might want to punt on using the 
> LSP watches:
>
> - File watching may not be implemented in the language server at all. That's 
> certainly the case for our hacked-up ycmd<->lsp bridge.


I guess you mean language client here, not language server.  In our case we 
already have a client capable of file watching, so it's convenient for us to do 
that (plus, this is what the LSP specification recommends).  If you want to 
make clangd capable of watching files natively, I am not against it, but it's 
not on our critical path.  As long as the file is watched, I'm happy.

Our client does not yet support file watch registration, that's why I did not 
implement it, but once we upgrade to a client that does, the plan is to make 
clangd ask the client to watch `**/compile_commands.json`.  Right now, we 
hardcoded in our client that we want to watch any file called 
`compile_commands.json`:

https://github.com/theia-ide/theia/pull/2405/commits/fe48e8d63f98edd879252238844a58f2cfa1

> - At some point we'll be interested in changes to the headers our sources 
> have included. This would involve a **lot** of watches for a dynamically 
> changing set of files. There's a good chance we'll have to consider using 
> native file watch APIs instead of the LSP methods to handle those (e.g., for 
> performance reasons).

What would be the performance bottleneck?  There would be a lot of watched 
files fore sure, but very little change events.

> We've been talking about starting to implement a better buildsystem 
> integration lately and taking compile_commands.json changes into account is 
> certainly on the table. We'll probably have some concrete proposals in 
> August/September.
> 
> In the meanwhile, maybe removing the caching of compile commands could fix 
> the problem? I don't have any data on how important the caching is to 
> performance (I would assume it to not be very important, since we only 
> request compile commands on the file changes; completion, findDefinitions, 
> etc reuse the last compile command anyway). 
>  https://reviews.llvm.org/D48071 added an option to disable the caching, it 
> never landed because we didn't agree on the default, but we can make it 
> happen.
>  It looks like a simpler workaround that works on all clients and we can 
> figure out how to properly integrate file watching later. WDYT?

... see answer below, as you have already provided an updated to this ...

In https://reviews.llvm.org/D49267#1171531, @ilya-biryukov wrote:

> @sammccall pointed out that I've been looking at a different layer of caching.
>  Clangd does per-directory (to avoid reloading compilation database multiple 
> times) and per-file (to avoid calling into compilation database multiple 
> times, it's expensive for our internal CDB) caching of compile commands.
>  https://reviews.llvm.org/D48071 allows to get rid of per-file cache, but 
> still keeps the per-directory cache, so this patch alone won't fix the 
> problem.


Indeed.

> We have a somewhat simple fix in mind: for interactive tools, like clangd, 
> the `JSONCompilationDatabase` could be configured to check for changes to 
> `compile_commands.json` and rebuild the compile commands on each call to its 
> methods, if necessary. E.g. we can still keep the per-directory "caching" of 
> `CompilationDatabase`s inside clangd, but CompilationDatabase instances will 
> start returning new compile commands when compile_commands.json is updated.
>  This would mean more FS accesses (and, potentially, json parsing) on 
> `getCompileCommands` and the same instance of `CompilationDatabase` will now 
> be potentially providing different compile commands for the same file on 
> different `getCompileCommands` calls.
>  However,
> 
> - The FS access concern shouldn't be noticeable for the clang tools: 
> parsing/preprocessing of files is usually done after the `getCompileCommands` 
> call, so the overhead of files access to compile_commands.json and json reads 
> should be negligible, compared to the work to process C/C++ files.
> - The fact that const object now changes seems to be a potential source of 
> confusion for the tools and (maybe?) some of them are not prepared for this, 
> so we should probably allow turning this behavior on and off. clangd (and 
> other interactive tools that might be interested in this), would turn it on, 
> while it will be off by default.
> 
>   It seems this could be implemented somewhere around the 
> `JSONCompilationDatabasePlugin` pretty easily. WDYT? I'm also happy to come 
> up with a patch to `JSONCompilationDatabasePlugin`, at this point I'm pretty 
> familiar with it. Unless you think this approach is flawed or want to do it 
> yourself, of course. In any 

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D49267#1173266, @ilya-biryukov wrote:

> The approach is not ideal, but may be a good middle ground before we figure 
> out how we approach file watching in clangd. Note that there are other things 
> that won't force the updates currently, e.g. changes to the included headers 
> won't cause rebuilds of the source files until user modifies them. And those 
> are much more frequent than changes to compile_commands.json, so they seem 
> like a more pressing problem.


Ok, I agree that having clangd watch files itself could be necessary at some 
point (when the client does not support it), but it would have to be 
configurable.  In our case, we have efficient enough file watching in the 
client, and already watch the files in the workspace.  Since the inotify 
watches are limited per-user, having clangd watch them too means we'll have 
duplicates, and therefore waste a rather limited resource.

Actually, clangd could simply use the client capabilities. If the client 
advertises support for file watching with dynamic registration, use that, 
otherwise use the internal mechanism.

In the mean time, I don't think the current patch paints us in a corner.  The 
logic of checking whether the effective compile commands for open files changes 
would stay even if the file watching mechanism changes.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49267



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

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:430
 CDB.clear();
-
-reparseOpenedFiles();
+compileCommandsChangePost(CCChangeData);
   }

malaperle wrote:
> ilya-biryukov wrote:
> > Maybe keep the old logic of reparsing all open files? This would make the 
> > change way simpler and I don't think we need this extra complexity in the 
> > long run, when we have better integration with the build system.
> > 
> > ClangdServer will reuse the preamble if compile command didn't change 
> > anyway, so reparse will be very fast and shouldn't be affected.
> > If the compile command does change, we'll retrigger the full rebuild.
> I think the change is not that complex but brings much added value. About the 
> integration with the build system, there are many build systems out there so 
> I don't think better integration will be useful in many scenarios (plain 
> make, custom builds, etc). This solution is generic enough so that any build 
> system that generates compile_commands.json will be supported in a pretty 
> good way.
@malaperle also suggested an alternative way of doing it.  Instead of saving 
the the compile commands in a custom structure before clearing the cache, we 
could save the compile flags in the `ParsedAST` object.  When some 
compile_commands.json changes, we can compare the new compile flags with this 
saved copy.  I think it would make the code a bit more straightforward.



Comment at: clangd/clients/clangd-vscode/src/extension.ts:35
 ['cpp', 'c', 'cc', 'cxx', 'c++', 'm', 'mm', 'h', 'hh', 'hpp', 'hxx', 
'inc'].join() + '}';
+const compileCommandsFilePattern: string = '**/compile_commands.json';
 const clientOptions: vscodelc.LanguageClientOptions = {

ilya-biryukov wrote:
> These watches apply to the children of the workspace root only, right?
> E.g., I quite often open the workspace in 
> 'llvm/tools/clang/tools/extra/clangd', would my `compile_commands.json`, 
> that's outside the workspace, be watched for changes?
You are right, I don't think files outside the workspace would get watched.

For your use case, we'll need clangd to watch (or ask the client to watch) the 
file.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49267



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

[PATCH] D49804: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name (take 2)

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
simark added reviewers: malaperle, ilya-biryukov, bkramer.

This is a new version of https://reviews.llvm.org/D48903, which has been 
reverted.  @ioeric fixed
the issues caused by this patch downstream, so he told me it was good to
go again.  I also fixed the test failures on Windows.  The issue is that
the paths returned by the InMemoryFileSystem directory iterators in the
tests mix posix and windows directory separators.  This is because we do
queries with posix-style separators ("/a/b") but filenames are appended
using native-style separators (backslash on Windows).  So we end up with
"/a/b\c".

I fixed the test by re-using the ReplaceBackslashes function defined in
another test.  I'm not sure this is the best fix, but the only
alternative I see would be to completely rewrite tests to use
posix-style paths on non-Windows and Windows-style paths on Windows.
That would lead to quite a bit of duplication...

Here's the original commit message:

InMemoryFileSystem::status behaves differently than
RealFileSystem::status.  The Name contained in the Status returned by
RealFileSystem::status will be the path as requested by the caller,
whereas InMemoryFileSystem::status returns the normalized path.

For example, when requested the status for "../src/first.h",
RealFileSystem returns a Status with "../src/first.h" as the Name.
InMemoryFileSystem returns "/absolute/path/to/src/first.h".

The reason for this change is that I want to make a unit test in the
clangd testsuite (where we use an InMemoryFileSystem) to reproduce a
bug I get with the clangd program (where a RealFileSystem is used).
This difference in behavior "hides" the bug in the unit test version.


Repository:
  rC Clang

https://reviews.llvm.org/D49804

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -151,6 +151,11 @@
 addEntry(Path, S);
   }
 };
+
+auto ReplaceBackslashes = [](std::string S) {
+  std::replace(S.begin(), S.end(), '\\', '/');
+  return S;
+};
 } // end anonymous namespace
 
 TEST(VirtualFileSystemTest, StatusQueries) {
@@ -782,7 +787,7 @@
 
   I = FS.dir_begin("/b", EC);
   ASSERT_FALSE(EC);
-  ASSERT_EQ("/b/c", I->getName());
+  ASSERT_EQ("/b/c", ReplaceBackslashes(I->getName()));
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
@@ -794,16 +799,12 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
 
-  auto ReplaceBackslashes = [](std::string S) {
-std::replace(S.begin(), S.end(), '\\', '/');
-return S;
-  };
   NormalizedFS.setCurrentWorkingDirectory("/b/c");
   NormalizedFS.setCurrentWorkingDirectory(".");
   ASSERT_EQ("/b/c", ReplaceBackslashes(
@@ -919,6 +920,37 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< Normal

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

I uploaded a new version of this patch here:
https://reviews.llvm.org/D49804


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D48903#1175317, @ioeric wrote:

> It would make it easier for your reviewers to look at the new changes if
>  you just reopen this patch and update the diff :)


I tried but didn't know how.  But I just saw the "Add Action" drop down with 
"Reopen" in it.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 157287.
simark added a comment.

Fix tests on Windows

Fix InMemoryFileSystem tests on Windows.  The paths returned by the
InMemoryFileSystem directory iterators in the tests mix posix and windows
directory separators.  THis is because we do queries with posix-style
separators ("/a/b") but filnames are appended using native-style separators
(backslash on Windows).  So we end up with "/a/b\c".

I fixed the test by re-using the ReplaceBackslashes function defined in another
test.  I'm not sure this is the best fix, but the only alternative I see would
be to completely rewrite tests to use posix-style paths on non-Windows and
Windows-style paths on Windows.  That would lead to quite a bit of
duplication...


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -151,6 +151,11 @@
 addEntry(Path, S);
   }
 };
+
+auto ReplaceBackslashes = [](std::string S) {
+  std::replace(S.begin(), S.end(), '\\', '/');
+  return S;
+};
 } // end anonymous namespace
 
 TEST(VirtualFileSystemTest, StatusQueries) {
@@ -782,7 +787,7 @@
 
   I = FS.dir_begin("/b", EC);
   ASSERT_FALSE(EC);
-  ASSERT_EQ("/b/c", I->getName());
+  ASSERT_EQ("/b/c", ReplaceBackslashes(I->getName()));
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
@@ -794,16 +799,12 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
 
-  auto ReplaceBackslashes = [](std::string S) {
-std::replace(S.begin(), S.end(), '\\', '/');
-return S;
-  };
   NormalizedFS.setCurrentWorkingDirectory("/b/c");
   NormalizedFS.setCurrentWorkingDirectory(".");
   ASSERT_EQ("/b/c", ReplaceBackslashes(
@@ -919,6 +920,37 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  ASSERT_EQ("../b/c", ReplaceBackslashes(It->getName()));
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -471,15 +471,31 @@
 /// The in memory file system is a tree of Nodes. Every node can either be a
 /// file or a directory.
 class InMemoryNode {
-  Status Stat;
   InMemoryNodeKind Kind;
+  Status Stat;
+
+protected:
+  /// Return Stat.  This should only be used for internal/debugging

[PATCH] D49783: [clangd] Do not rebuild AST if inputs have not changed

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Thanks, that's simple and efficient.  I'll update 
https://reviews.llvm.org/D49267 (to call `reparseOpenFiles` once again) once 
this is merged.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49783



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark marked 7 inline comments as done.
simark added inline comments.



Comment at: lib/Basic/VirtualFileSystem.cpp:475
   InMemoryNodeKind Kind;
+  Status Stat;
+

ilya-biryukov wrote:
> NIT: maybe keep the order of members the same to keep the patch more focused? 
> Unless there's a good reason to swap them.
Oops, that was not intended.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 157467.
simark marked an inline comment as done.
simark added a comment.

I think this addresses all of Ilya's comments.


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -13,6 +13,7 @@
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"
 #include 
@@ -151,6 +152,13 @@
 addEntry(Path, S);
   }
 };
+
+/// Replace back-slashes by front-slashes.
+std::string getPosixPath(std::string S) {
+  SmallString<128> Result;
+  llvm::sys::path::native(S, Result, llvm::sys::path::Style::posix);
+  return Result.str();
+};
 } // end anonymous namespace
 
 TEST(VirtualFileSystemTest, StatusQueries) {
@@ -782,7 +790,9 @@
 
   I = FS.dir_begin("/b", EC);
   ASSERT_FALSE(EC);
-  ASSERT_EQ("/b/c", I->getName());
+  // When on Windows, we end up with "/b\\c" as the name.  Convert to Posix
+  // path for the sake of the comparison.
+  ASSERT_EQ("/b/c", getPosixPath(I->getName()));
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
@@ -794,23 +804,19 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
 
-  auto ReplaceBackslashes = [](std::string S) {
-std::replace(S.begin(), S.end(), '\\', '/');
-return S;
-  };
   NormalizedFS.setCurrentWorkingDirectory("/b/c");
   NormalizedFS.setCurrentWorkingDirectory(".");
-  ASSERT_EQ("/b/c", ReplaceBackslashes(
-NormalizedFS.getCurrentWorkingDirectory().get()));
+  ASSERT_EQ("/b/c",
+getPosixPath(NormalizedFS.getCurrentWorkingDirectory().get()));
   NormalizedFS.setCurrentWorkingDirectory("..");
-  ASSERT_EQ("/b", ReplaceBackslashes(
-  NormalizedFS.getCurrentWorkingDirectory().get()));
+  ASSERT_EQ("/b",
+getPosixPath(NormalizedFS.getCurrentWorkingDirectory().get()));
 }
 
 #if !defined(_WIN32)
@@ -919,6 +925,39 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  // When on Windows, we end up with "../b\\c" as the name.  Convert to Posix
+  // path for the sake of the comparison.
+  ASSERT_EQ("../b/c", getPosixPath(It->getName()));
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: lib/Basic/VirtualFileSystem.cpp
=

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC338057: [VirtualFileSystem] InMemoryFileSystem::status: 
Return a Status with the… (authored by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48903?vs=157467&id=157548#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -315,9 +315,11 @@
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
-  if (UFE.File)
-if (auto RealPathName = UFE.File->getName())
-  UFE.RealPathName = *RealPathName;
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE.RealPathName = RealPathName.str();
+
   return &UFE;
 }
 
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -474,12 +474,28 @@
   Status Stat;
   InMemoryNodeKind Kind;
 
+protected:
+  /// Return Stat.  This should only be used for internal/debugging use.  When
+  /// clients wants the Status of this node, they should use
+  /// \p getStatus(StringRef).
+  const Status &getStatus() const { return Stat; }
+
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
   : Stat(std::move(Stat)), Kind(Kind) {}
   virtual ~InMemoryNode() = default;
 
-  const Status &getStatus() const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+
+  /// Get the filename of this node (the name without the directory part).
+  StringRef getFileName() const {
+return llvm::sys::path::filename(Stat.getName());
+  }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -504,14 +520,21 @@
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile &Node;
+  /// The name to use when returning a Status for this file.
+  std::string RequestedName;
 
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile &Node) : Node(Node) {}
+  explicit InMemoryFileAdaptor(InMemoryFile &Node, std::string RequestedName)
+  : Node(Node), RequestedName(std::move(RequestedName)) {}
 
-  llvm::ErrorOr status() override { return Node.getStatus(); }
+  llvm::ErrorOr status() override {
+return Node.getStatus(RequestedName);
+  }
 
   llvm::ErrorOr>
   getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +734,7 @@
 llvm::ErrorOr InMemoryFileSystem::status(const Twine &Path) {
   auto Node = lookupInMemoryNode(*this, Root.get(), Path);
   if (Node)
-return (*Node)->getStatus();
+return (*Node)->getStatus(Path.str());
   return Node.getError();
 }
 
@@ -724,7 +747,8 @@
   // When we have a file provide a heap-allocated wrapper for the memory buffer
   // to match the ownership semantics for File.
   if (auto *F = dyn_cast(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(
+new detail::InMemoryFileAdaptor(*F, Path.str()));
 
   // FIXME: errc::not_a_file?
   return make_error_code(llvm::errc::invalid_argument);
@@ -736,21 +760,33 @@
 class InMemoryDirIterator : public clang::vfs::detail::DirIterImpl {
   detail::InMemoryDirectory::const_iterator I;
   detail::InMemoryDirectory::const_iterator E;
+  std::string RequestedDirName;
+
+  void setCurrentEntry() {
+if (I != E) {
+  SmallString<256> Path(RequestedDirName);
+  llvm::sys::path::append(Path, I->second->getFileName());
+  CurrentEntry = I->second->getStatus(Path);
+} else {
+  // When we're at the end, make CurrentEntry invalid and DirIterImpl will
+  // do the rest.
+  CurrentEntry = Status();
+}
+  }
 
 public:
   InMemoryDirIterator() = default;
 
-  explicit InMemoryDirIterator(detail::InMemoryDirectory &Dir)
-  : I(Dir.begin()), E(Dir.end()) {
-if (I != E)
-  CurrentEntry = I->second->getStatus();
+  explicit InMemoryDirIterator(detail::InMemoryDirectory &Dir,
+   std::string RequestedDirName)
+  : I(Dir.begin()), E(Dir.end()),
+RequestedDirName(std::move(RequestedDirName)) {
+setCurrentEntry();
   }
 
   std::error

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D49833#1182615, @malaperle wrote:

> @simark would you mind finishing this patch and committing it? I won't be 
> able to finish it given that I started it at a different company, etc.


Yes, I'll take it over, thanks for getting it started.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49833



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

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 158366.
simark added a comment.

Address Ilya's comment (add an indirection for initializationOptions).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49833

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h

Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -322,6 +322,16 @@
 
 bool fromJSON(const llvm::json::Value &, ClientCapabilities &);
 
+/// Clangd extension to set clangd-specific "initializationOptions" in the
+/// "initialize" request and for the "workspace/didChangeConfiguration"
+/// notification since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
+
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {};
+
 struct InitializeParams {
   /// The process Id of the parent process that started
   /// the server. Is null if the process has not been started by another
@@ -348,6 +358,10 @@
 
   /// The initial trace setting. If omitted trace is disabled ('off').
   llvm::Optional trace;
+
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  llvm::Optional initializationOptions;
 };
 bool fromJSON(const llvm::json::Value &, InitializeParams &);
 
@@ -419,13 +433,6 @@
 };
 bool fromJSON(const llvm::json::Value &, DidChangeWatchedFilesParams &);
 
-/// Clangd extension to manage a workspace/didChangeConfiguration notification
-/// since the data received is described as 'any' type in LSP.
-struct ClangdConfigurationParamsChange {
-  llvm::Optional compilationDatabasePath;
-};
-bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
-
 struct DidChangeConfigurationParams {
   // We use this predefined struct because it is easier to use
   // than the protocol specified type of 'any'.
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -263,7 +263,7 @@
   O.map("rootPath", R.rootPath);
   O.map("capabilities", R.capabilities);
   O.map("trace", R.trace);
-  // initializationOptions, capabilities unused
+  O.map("initializationOptions", R.initializationOptions);
   return true;
 }
 
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -82,6 +82,7 @@
   /// may be very expensive.  This method is normally called when the
   /// compilation database is changed.
   void reparseOpenedFiles();
+  void applyConfiguration(const ClangdConfigurationParamsChange &Settings);
 
   JSONOutput &Out;
   /// Used to indicate that the 'shutdown' request was received from the
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -72,6 +72,9 @@
 } // namespace
 
 void ClangdLSPServer::onInitialize(InitializeParams &Params) {
+  if (Params.initializationOptions)
+applyConfiguration(*Params.initializationOptions);
+
   if (Params.rootUri && *Params.rootUri)
 Server.setRootPath(Params.rootUri->file());
   else if (Params.rootPath && !Params.rootPath->empty())
@@ -398,11 +401,8 @@
});
 }
 
-// FIXME: This function needs to be properly tested.
-void ClangdLSPServer::onChangeConfiguration(
-DidChangeConfigurationParams &Params) {
-  ClangdConfigurationParamsChange &Settings = Params.settings;
-
+void ClangdLSPServer::applyConfiguration(
+const ClangdConfigurationParamsChange &Settings) {
   // Compilation database change.
   if (Settings.compilationDatabasePath.hasValue()) {
 NonCachedCDB.setCompileCommandsDir(
@@ -413,6 +413,13 @@
   }
 }
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(
+DidChangeConfigurationParams &Params) {
+  ClangdConfigurationParamsChange &Settings = Params.settings;
+  applyConfiguration(Settings);
+}
+
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out,
  const clangd::CodeCompleteOptions &CCOpts,
  llvm::Optional CompileCommandsDir,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL338518: [clangd] Receive compilationDatabasePath in 
'initialize' request (authored by simark, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49833?vs=158366&id=158503#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49833

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h

Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h
@@ -82,6 +82,7 @@
   /// may be very expensive.  This method is normally called when the
   /// compilation database is changed.
   void reparseOpenedFiles();
+  void applyConfiguration(const ClangdConfigurationParamsChange &Settings);
 
   JSONOutput &Out;
   /// Used to indicate that the 'shutdown' request was received from the
Index: clang-tools-extra/trunk/clangd/Protocol.cpp
===
--- clang-tools-extra/trunk/clangd/Protocol.cpp
+++ clang-tools-extra/trunk/clangd/Protocol.cpp
@@ -263,7 +263,7 @@
   O.map("rootPath", R.rootPath);
   O.map("capabilities", R.capabilities);
   O.map("trace", R.trace);
-  // initializationOptions, capabilities unused
+  O.map("initializationOptions", R.initializationOptions);
   return true;
 }
 
Index: clang-tools-extra/trunk/clangd/Protocol.h
===
--- clang-tools-extra/trunk/clangd/Protocol.h
+++ clang-tools-extra/trunk/clangd/Protocol.h
@@ -322,6 +322,16 @@
 
 bool fromJSON(const llvm::json::Value &, ClientCapabilities &);
 
+/// Clangd extension to set clangd-specific "initializationOptions" in the
+/// "initialize" request and for the "workspace/didChangeConfiguration"
+/// notification since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
+
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {};
+
 struct InitializeParams {
   /// The process Id of the parent process that started
   /// the server. Is null if the process has not been started by another
@@ -348,6 +358,10 @@
 
   /// The initial trace setting. If omitted trace is disabled ('off').
   llvm::Optional trace;
+
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  llvm::Optional initializationOptions;
 };
 bool fromJSON(const llvm::json::Value &, InitializeParams &);
 
@@ -419,13 +433,6 @@
 };
 bool fromJSON(const llvm::json::Value &, DidChangeWatchedFilesParams &);
 
-/// Clangd extension to manage a workspace/didChangeConfiguration notification
-/// since the data received is described as 'any' type in LSP.
-struct ClangdConfigurationParamsChange {
-  llvm::Optional compilationDatabasePath;
-};
-bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
-
 struct DidChangeConfigurationParams {
   // We use this predefined struct because it is easier to use
   // than the protocol specified type of 'any'.
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -72,6 +72,9 @@
 } // namespace
 
 void ClangdLSPServer::onInitialize(InitializeParams &Params) {
+  if (Params.initializationOptions)
+applyConfiguration(*Params.initializationOptions);
+
   if (Params.rootUri && *Params.rootUri)
 Server.setRootPath(Params.rootUri->file());
   else if (Params.rootPath && !Params.rootPath->empty())
@@ -398,11 +401,8 @@
});
 }
 
-// FIXME: This function needs to be properly tested.
-void ClangdLSPServer::onChangeConfiguration(
-DidChangeConfigurationParams &Params) {
-  ClangdConfigurationParamsChange &Settings = Params.settings;
-
+void ClangdLSPServer::applyConfiguration(
+const ClangdConfigurationParamsChange &Settings) {
   // Compilation database change.
   if (Settings.compilationDatabasePath.hasValue()) {
 NonCachedCDB.setCompileCommandsDir(
@@ -413,6 +413,12 @@
   }
 }
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(
+DidChangeConfigurationParams &Params) {
+  applyConfiguration(Params.settings);
+}
+
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out,
  const clangd::CodeCompleteOptions &CCOpts,
  llvm::Optional CompileCommandsDir,
__

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Simon Marchi via Phabricator via cfe-commits]

simark marked 2 inline comments as done.
simark added a comment.

Thanks, done.


Repository:
  rL LLVM

https://reviews.llvm.org/D49833



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a subscriber: eric_niebler.
simark added a comment.

@eric_niebler, question for you:

This patch causes clang to emit a `-Wnonportable-include-path` warning where it 
did not before.  It affects the following test on Windows:
https://github.com/llvm-mirror/clang/blob/master/test/PCH/case-insensitive-include.c

The warning is currently not emitted for the **last** clang invocation (the one 
that includes PCH), because the real path value is not available, and therefore 
this condition is false:
https://github.com/llvm-mirror/clang/blob/fe1098c84823b8eac46b0bfffc5f5788b6c26d1a/lib/Lex/PPDirectives.cpp#L2015

With this patch, the real path value is available, so we emit the warning.  Is 
it on purpose that this warning is not emitted in this case?  If not should I 
simply add `-Wno-nonportable-include-path` to the last clang invocation, as was 
done earlier with the first invocation?

It seems to me like the warning is valid, even though we use precompiled 
headers.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 158625.
simark added a comment.

Add -Wno-nonportable-include-path to test/PCH/case-insensitive-include.c


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  test/PCH/case-insensitive-include.c
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -13,6 +13,7 @@
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"
 #include 
@@ -151,6 +152,13 @@
 addEntry(Path, S);
   }
 };
+
+/// Replace back-slashes by front-slashes.
+std::string getPosixPath(std::string S) {
+  SmallString<128> Result;
+  llvm::sys::path::native(S, Result, llvm::sys::path::Style::posix);
+  return Result.str();
+};
 } // end anonymous namespace
 
 TEST(VirtualFileSystemTest, StatusQueries) {
@@ -782,7 +790,9 @@
 
   I = FS.dir_begin("/b", EC);
   ASSERT_FALSE(EC);
-  ASSERT_EQ("/b/c", I->getName());
+  // When on Windows, we end up with "/b\\c" as the name.  Convert to Posix
+  // path for the sake of the comparison.
+  ASSERT_EQ("/b/c", getPosixPath(I->getName()));
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
@@ -794,23 +804,19 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
 
-  auto ReplaceBackslashes = [](std::string S) {
-std::replace(S.begin(), S.end(), '\\', '/');
-return S;
-  };
   NormalizedFS.setCurrentWorkingDirectory("/b/c");
   NormalizedFS.setCurrentWorkingDirectory(".");
-  ASSERT_EQ("/b/c", ReplaceBackslashes(
-NormalizedFS.getCurrentWorkingDirectory().get()));
+  ASSERT_EQ("/b/c",
+getPosixPath(NormalizedFS.getCurrentWorkingDirectory().get()));
   NormalizedFS.setCurrentWorkingDirectory("..");
-  ASSERT_EQ("/b", ReplaceBackslashes(
-  NormalizedFS.getCurrentWorkingDirectory().get()));
+  ASSERT_EQ("/b",
+getPosixPath(NormalizedFS.getCurrentWorkingDirectory().get()));
 }
 
 #if !defined(_WIN32)
@@ -919,6 +925,39 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  // When on Windows, we end up with "../b\\c" as the name.  Convert to Posix
+  // path for the sake of the comparison.
+  ASSERT_EQ("../b/c", getPosixPath(It->getName()));
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: test/PCH/c

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

That's fine with me.  I'll try it and time will tell if I like it or not :).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50154



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

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D50154#1185605, @sammccall wrote:

> Capitalizing sounds nice. +1 to just do it without an option.
>
> My favorite essay on this is
>  http://neugierig.org/software/blog/2018/07/options.html


Agreed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50154



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

[PATCH] D50255: Add test for changing build configuration

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, arphaman, jkorous, ioeric, ilya-biryukov.

This patch adds tests for the two ways of changing build configuration
(pointing to a particular compile_commands.json):

- Through the workspace/didChangeConfiguration notification.
- Through the initialize request.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50255

Files:
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/compile-commands-path.test


Index: test/clangd/compile-commands-path.test
===
--- /dev/null
+++ test/clangd/compile-commands-path.test
@@ -0,0 +1,37 @@
+# Test that we can switch between configuration/build using the
+# workspace/didChangeConfiguration notification.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+# RUN: sed -e "s|INPUT_DIR|%t.dir|g" %s > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is not defined",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is one",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is two",
+---
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: test/clangd/compile-commands-path-in-initialize.test
===
--- /dev/null
+++ test/clangd/compile-commands-path-in-initialize.test
@@ -0,0 +1,30 @@
+# Test that we can set choose a configuration/build directly in the initialize
+# request.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+# RUN: sed -e "s|INPUT_DIR|%t.dir|g" %s > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"initializationOptions":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is one",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is two",
+---
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}


Index: tes

[PATCH] D50255: Add test for changing build configuration

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

This was not tested on windows yet.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50255



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark requested review of this revision.
simark added a comment.

In https://reviews.llvm.org/D48903#1187596, @ilya-biryukov wrote:

> This revision got 'reopened' and is now in the list of accepted revision. 
> Should we close it again?


It got reverted a second time because it was breaking a test on Windows.  The 
new bit is the change to `test/PCH/case-insensitive-include.c`, so it would 
need review again.  If somebody else could run the tests on Windows, it would 
make me a bit more confident too.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D50255: Add test for changing build configuration

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 159050.
simark added a comment.

The tests now work on Windows, I added some path adjustment steps.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50255

Files:
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/compile-commands-path.test


Index: test/clangd/compile-commands-path.test
===
--- /dev/null
+++ test/clangd/compile-commands-path.test
@@ -0,0 +1,42 @@
+# Test that we can switch between configuration/build using the
+# workspace/didChangeConfiguration notification.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+
+# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
+
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is not defined",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is one",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is two",
+---
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: test/clangd/compile-commands-path-in-initialize.test
===
--- /dev/null
+++ test/clangd/compile-commands-path-in-initialize.test
@@ -0,0 +1,35 @@
+# Test that we can set choose a configuration/build directly in the initialize
+# request.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+
+# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
+
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"initializationOptions":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is one",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
+# CHECK:   "m

[PATCH] D50255: Add test for changing build configuration

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL338914: [clangd] Add test for changing build configuration 
(authored by simark, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D50255

Files:
  clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
  clang-tools-extra/trunk/test/clangd/compile-commands-path.test


Index: 
clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
===
--- clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
+++ clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
@@ -0,0 +1,35 @@
+# Test that we can set choose a configuration/build directly in the initialize
+# request.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+
+# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
+
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"initializationOptions":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is one",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is two",
+---
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: clang-tools-extra/trunk/test/clangd/compile-commands-path.test
===
--- clang-tools-extra/trunk/test/clangd/compile-commands-path.test
+++ clang-tools-extra/trunk/test/clangd/compile-commands-path.test
@@ -0,0 +1,42 @@
+# Test that we can switch between configuration/build using the
+# workspace/didChangeConfiguration notification.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+
+# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
+
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is not defined",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+# CHECK:  

[PATCH] D50255: Add test for changing build configuration

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D50255#1187871, @arphaman wrote:

> Thanks! This LGTM.


Well, thanks for the hints!


Repository:
  rL LLVM

https://reviews.llvm.org/D50255



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D48903#1188566, @malaperle wrote:

> Both check-clang and check-clang-tools pass successfully for me on Windows 
> with the patch.


Awesome, thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 133034.
simark added a comment.

Fix assertion about parsing a document that is not open

As found by Ilya, the getActiveFiles method would return the documents that 
were previously opened and then closed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h

Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -51,6 +51,7 @@
   virtual void onCommand(ExecuteCommandParams &Params) = 0;
   virtual void onRename(RenameParams &Parames) = 0;
   virtual void onDocumentHighlight(TextDocumentPositionParams &Params) = 0;
+  virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -71,4 +71,6 @@
   Register("workspace/executeCommand", &ProtocolCallbacks::onCommand);
   Register("textDocument/documentHighlight",
&ProtocolCallbacks::onDocumentHighlight);
+  Register("workspace/didChangeConfiguration",
+   &ProtocolCallbacks::onChangeConfiguration);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -265,6 +265,20 @@
 };
 bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &);
 
+/// Clangd extension to manage a workspace/didChangeConfiguration notification
+/// since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const json::Expr &, ClangdConfigurationParamsChange &);
+
+struct DidChangeConfigurationParams {
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  ClangdConfigurationParamsChange settings;
+};
+bool fromJSON(const json::Expr &, DidChangeConfigurationParams &);
+
 struct FormattingOptions {
   /// Size of a tab in spaces.
   int tabSize;
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -400,5 +400,15 @@
   };
 }
 
+bool fromJSON(const json::Expr &Params, DidChangeConfigurationParams &CCP) {
+  json::ObjectMapper O(Params);
+  return O && O.map("settings", CCP.settings);
+}
+
+bool fromJSON(const json::Expr &Params, ClangdConfigurationParamsChange &CCPC) {
+  json::ObjectMapper O(Params);
+  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath);
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -61,6 +61,9 @@
   /// Uses the default fallback command, adding any extra flags.
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
+  /// Set the compile commands directory to \p P.
+  void setCompileCommandsDir(Path P);
+
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
 
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -51,6 +51,12 @@
   return C;
 }
 
+void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(Path P) {
+  std::lock_guard Lock(Mutex);
+  CompileCommandsDir = P;
+  CompilationDatabases.clear();
+}
+
 void DirectoryBasedGlobalCompilationDatabase::setExtraFlagsForFile(
 PathRef File, std::vector ExtraFlags) {
   std::lock_guard Lock(Mutex);
Index: clangd/DraftStore.h
===
--- clangd/DraftStore.h
+++ clangd/DraftStore.h
@@ -40,6 +40,12 @@
   /// \return version and contents of the stored document.
   /// For untracked files, a (0, None) pair is returned.
   VersionedDraft getDraft(PathRef File) const;
+
+  /// \return List of names of active drafts in this store.  Drafts that were
+  /// removed (which still have an entry in the Drafts map) are not returned by
+  /// this function.
+  std::vector getActiveFiles() const;
+
   /// \return version of the tracked document.
   /// For untracked files, 0 is returned.
   DocVersion getVersion(PathRef File) const;
Index: clangd/DraftStore.cpp
===

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 133367.
simark added a comment.
Herald added subscribers: ioeric, jkorous-apple.

New version

Here's a new version of the patch, quite reworked.  I scaled down the scope a
little bit to make it simpler (see commit log).  We'll add more features later
on.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -227,6 +227,316 @@
   }
 }
 
+TEST(Hover, All) {
+
+  struct OneTest {
+const char *input;
+const char *expectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },
+  {
+  R"cpp(// Typedef
+typedef int Foo;
+int main() {
+  ^Foo bar;
+}
+  )cpp",
+  "Declared in global namespace\n\ntypedef int Foo",
+  },
+  {
+  R"cpp(// Namespace
+namespace ns {
+struct Foo { static 

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D35894#1001875, @ilya-biryukov wrote:

> Thanks for picking this up!
>  I have just one major comment related to how we generate the hover text.
>
> Current implementation tries to get the actual source code for every 
> declaration and then patch it up to look nice on in the output.
>  It is quite a large piece of code (spanning most of the patch) and tries to 
> mix and match source code and output from the AST when there's not enough 
> information in the AST (e.g. `tryFixupIndentation` and 
> `stripOpeningBracket`). This approach is really complex as we'll inevitably 
> try to parse "parts" of C++ and figure out how to deal with macros, etc.
>  Worse than that, I would expect this code to grow uncontrollably with time 
> and be very hard to maintain.
>  Moreover, in presence of macros it arguably gives non-useful results. For 
> example, consider this:
>
>   #define DEFINITIONS int foo(); int bar(); int baz();
>  
>   // somewhere else
>   DEFINITIONS
>  
>   // somewhere else
>   int test() {
> foo(); // <-- hover currently doesn't work here. And even if it did, 
> showing a line with just DEFINITIONS is not that useful.
>   }
>
>
> I suggest we move to a different approach of pretty-printing the relevant 
> parts of the AST. It is already implemented in clang, handles all the cases 
> in the AST (and will be updated along when AST is changed), shows useful 
> information in presence of macros and is much easier to implement.
>  The version of `getHoverContents` using this is just a few lines of code:
>
>   static std::string getHoverContents(const Decl *D) {
> if (TemplateDecl *TD = D->getDescribedTemplate())
>   D = TD; // we want to see the template bits.
>  
> std::string Result;
> llvm::raw_string_ostream OS(Result);
>  
> PrintingPolicy Policy(D->getASTContext().getLangOpts());
> Policy.TerseOutput = true;
> D->print(OS, Policy);
>  
> OS.flush();
> return Result;
>   }
>
>
> It doesn't add the `"Defined in ..."` piece, but illustrates the APIs we can 
> use. We should use the same APIs for the scope as well, avoiding fallbacks to 
> manual printing if we can.
>  If there's something missing/wrong in the upstream pretty printers, it's 
> fine and we can fix them (e.g., the thing that I've immediately noticed is 
> that namespaces are always printed with curly braces).


I thought it would be nice to show in the hover the declaration formatted as it 
is written in the source file, because that's how the developer is used to read 
it.  But on the other hand, I completely agree that trying to do string 
formatting ourselves is opening a can of worms.  I'll try the approach you 
suggest.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: unittests/clangd/XRefsTests.cpp:231
+TEST(Hover, All) {
+
+  struct OneTest {

ilya-biryukov wrote:
> NIT: remove empty line at the start of the function
Done.



Comment at: unittests/clangd/XRefsTests.cpp:233
+  struct OneTest {
+const char *input;
+const char *expectedHover;

ilya-biryukov wrote:
> NIT: Use `StringRef` instead of `const char*`
Done.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:325
+void ClangdLSPServer::onHover(TextDocumentPositionParams &Params) {
+
+  llvm::Expected> H =

ilya-biryukov wrote:
> NIT: remove the empty line at the start of function
Done.



Comment at: clangd/Protocol.cpp:324
+json::Expr toJSON(const MarkupContent &MC) {
+  const char *KindStr = NULL;
+

ilya-biryukov wrote:
> NIT: use nullptr instead of NULL
> (irrelevant if we use `StringRef`, see other comment below)
Ack.



Comment at: clangd/Protocol.cpp:331
+
+  switch (MC.kind) {
+  case MarkupKind::PlainText:

ilya-biryukov wrote:
> - `StringRef` is a nicer abstraction, maybe use it instead of `const char*`?
> - Maybe move declaration of `KindStr` closer to its usage?
> - Maybe extract a helper function to mark the code path for invalid kinds as 
> unreachable? I.e.
> ```
> static StringRef toTextKind(MarkupKind Kind) {
>   switch (Kind) {
> case MarkupKind::PlainText:
>   return "plaintext";
> case MarkupKind::Markdown:
>   return "markdown";
>   }
>   llvm_unreachable("Invalid MarkupKind");
> }
> ```
Done.



Comment at: clangd/XRefs.cpp:326
+  if (const TagDecl *TD = dyn_cast(ND)) {
+switch (TD->getTagKind()) {
+case TTK_Class:

ilya-biryukov wrote:
> Same suggestion as before. Could we extract a helper function to mark invalid 
> enum values unreachable?
> 
Done.



Comment at: clangd/XRefs.cpp:552
+  if (!Decls.empty()) {
+assert(Decls[0] != nullptr);
+if (Decls[0] != nullptr)

ilya-biryukov wrote:
> This assert seems rather unlikely and just makes the code more complex. Let's 
> assume it's true and remove it (along with the subsequent if statement)
Done...



Comment at: clangd/XRefs.cpp:560
+assert(Macros[0] != nullptr);
+if (Macros[0] != nullptr)
+  return getHoverContents(Macros[0], AST);

ilya-biryukov wrote:
> This assert seems rather unlikely and just makes the code more complex. Let's 
> assume it's true and remove it (along with the subsequent if statement)
> 
> 
... and done.



Comment at: test/clangd/hover.test:2
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#

ilya-biryukov wrote:
> We have a more readable format for the lit-tests now. Could we update this 
> test to use it?
> (see other tests in test/clangd for example)
Indeed the new format looks much more readable.  I'll modify this one to look 
like completion.test.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Another example where pretty-printing the AST gives better results:

  int x, y = 5, z = 6;

Hover the `z` will now show `int z = 6`, before it would have shown `int x, y = 
5, z = 6`.  I think new version is better because it only shows the variable we 
care about.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

The only problem I see is that when hovering a function name, it now prints the 
whole function definition.  When talking with @malaperle, he told me that you 
had discussed it before and we should not have the definition in the hover, 
just the prototype.  Glancing quickly at the PrintingPolicy, I don't see a way 
to just print the prototype of the function.  Do you have any idea?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D35894#1003342, @simark wrote:

> The only problem I see is that when hovering a function/struct name, it now 
> prints the whole function/struct definition.  When talking with @malaperle, 
> he told me that you had discussed it before and we should not have the 
> definition in the hover, just the prototype for a function and the name (e.g. 
> "struct foo") for a struct.  Glancing quickly at the PrintingPolicy, I don't 
> see a way to do that.  Do you have any idea?


Arrg, Sorry, I just re-read your comment and saw that you used TerseOutput 
which does that.  I did not catch that when transcribing the code (I did not 
want to copy paste to understand the code better, but got bitten).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

> I'm not aware of the code that pretty-prints macros. There's a code that 
> dumps debug output, but it's not gonna help in that case.
>  Alternatively, we could simply print the macro name and not print the 
> definition. That's super-simple and is in line with hovers for the AST decls. 
> Let's do that in the initial version.

Ok, I'm considering printing "#define MACRO".  Just printing the name would not 
be very useful, at least printing "#define" gives the information that the 
hovered entity is defined as a macro (and not a function or variable).

Is there a way to get the macro name from the MacroInfo object?  I couldn't 
find any, so the solution I'm considering is to make 
`DeclarationAndMacrosFinder::takeMacroInfos` return an 
`std::vector>`, where the first member 
of the pair is the macro name.  It would come from `IdentifierInfo->getName()` 
in `DeclarationAndMacrosFinder::finish`.  Does that make sense, or is there a 
simpler way?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

I think I managed to make some tests by using the `MockCompilationDatabase`.  
Basically with some code like:

  #ifndef MACRO
  static void func () {}  // 1
  #else
  static void func () {}  // 2
  #endif

and these steps:

1. Server.addDocument(...)
2. Server.findDefinitions (assert that it returns definition 1)
3. CDB.ExtraClangFlags.push_back("-DMACRO=1")
4. Server.reparseOpenedFiles()
5. Server.findDefinitions (assert that it returns definition 2)

Right now that test fails, but it's not clear to me whether it's because the 
test is wrong or there's really a bug in there.  I'll upload a work-in-progress 
version.




Comment at: clangd/ClangdServer.cpp:541
 
+std::vector>
+ClangdServer::reparseOpenedFiles() {

ilya-biryukov wrote:
> We're not returning futures from `forceReparse` anymore, this function has to 
> be updated accordingly.
Indeed, I found this by rebasing.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 134097.
simark added a comment.

Add tests, work in progress

Can you take a look at the "TEST(DidChangeConfiguration, DifferentDeclaration)"
test, and tell me if you see anything wrong with it?  It seems to me like
changes in command line arguments (adding -DMACRO=1) are not taken into account
when changing configuration.

The other test (also very much work-in-progress) would be to verify that
changing configuration gets us the right diagnostics.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -257,6 +257,86 @@
SourceAnnotations.range()}));
 }
 
+TEST(DidChangeConfiguration, DifferentDeclaration) {
+  Annotations SourceAnnotations(R"cpp(
+#ifndef MACRO
+$before[[static void bob() {}]]
+#else
+$after[[static void bob() {}]]
+#endif
+
+int main() { b^ob(); }
+)cpp");
+
+  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
+  MockFSProvider FS;
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  FS.Files[FooCpp] = "";
+
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS, /*AsyncThreadsCount=*/0,
+  /*StorePreambleInMemory=*/true);
+
+  Server.addDocument(FooCpp, SourceAnnotations.code());
+
+  /* Without MACRO defined.  */
+  auto Locations = Server.findDefinitions(FooCpp, SourceAnnotations.point());
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{URIForFile{FooCpp.str()},
+   SourceAnnotations.range("before")}));
+
+  CDB.ExtraClangFlags.push_back("-DMACRO=1");
+  Server.reparseOpenedFiles();
+
+  /* With MACRO defined.  */
+  Locations = Server.findDefinitions(FooCpp, SourceAnnotations.point());
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{URIForFile{FooCpp.str()},
+   SourceAnnotations.range("after")}));
+}
+
+class MyDiags : public DiagnosticsConsumer {
+  void onDiagnosticsReady(
+  PathRef File, Tagged> Diagnostics) override {
+std::cout << "There are " << Diagnostics.Value.size() << std::endl;
+for (DiagWithFixIts &Diag : Diagnostics.Value) {
+  std::cout << "  " << Diag.Diag.message << std::endl;
+}
+  }
+};
+
+TEST(DidChangeConfiguration, Diagnostics) {
+  Annotations SourceAnnotations(R"cpp(
+#ifdef WITH_ERROR
+this is an error
+#endif
+
+int main() { return 0; }
+)cpp");
+
+  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
+  CDB.ExtraClangFlags.push_back("-DWITH_ERROR=1");
+  MockFSProvider FS;
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  FS.Files[FooCpp] = "";
+
+  MyDiags DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS, /*AsyncThreadsCount=*/0,
+  /*StorePreambleInMemory=*/true);
+
+  Server.addDocument(FooCpp, SourceAnnotations.code());
+
+
+  CDB.ExtraClangFlags.clear();
+
+  Server.reparseOpenedFiles();
+
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -51,6 +51,7 @@
   virtual void onCommand(ExecuteCommandParams &Params) = 0;
   virtual void onRename(RenameParams &Parames) = 0;
   virtual void onDocumentHighlight(TextDocumentPositionParams &Params) = 0;
+  virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -71,4 +71,6 @@
   Register("workspace/executeCommand", &ProtocolCallbacks::onCommand);
   Register("textDocument/documentHighlight",
&ProtocolCallbacks::onDocumentHighlight);
+  Register("workspace/didChangeConfiguration",
+   &ProtocolCallbacks::onChangeConfiguration);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -265,6 +265,20 @@
 };
 bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &);
 
+/// Clangd extension to manage a workspace/didChangeConfiguration notification
+/// since the data received is 

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D39571#1007291, @ilya-biryukov wrote:

> It looks like a bug in the preamble handling. (It does not check if macros 
> were redefined).
>  You can workaround that by making sure the preamble ends before your code 
> starts (preamble only captures preprocessor directives, so any C++ decl will 
> end it):
>
> Annotations SourceAnnotations(R"cpp(
>   int avoid_preamble;
>  
>   #ifndef MACRO
>   $before[[static void bob() {}]]
>   #else
>   $after[[static void bob() {}]]
>   #endif
>   /// 
>   )cpp"
>


Ah ok, indeed this test now works when I add this.

The other test I am working on (at the bottom of the file) acts weird, even if 
I add `int avoid_preamble`.  The idea of the test is:

1. Set -DWITH_ERROR=1 in the compile commands database
2. Add the document, expect one error diagnostic
3. Unset WITH_ERROR in the compile commands database
4. Reparse the file, expect no error diagnostic

If I do it in this order, I get one diagnostic both times, when I don't expect 
one the second time the file is parsed.  But if I do it the other way (first 
parse with no errors, second parse with an error), it works fine.




Comment at: unittests/clangd/XRefsTests.cpp:260
 
+TEST(DidChangeConfiguration, DifferentDeclaration) {
+  Annotations SourceAnnotations(R"cpp(

ilya-biryukov wrote:
> I'd move it to `ClangdTests.cpp`, generic `ClangdServer` tests usually go 
> there. It's fine to `#include "Annotations.h"` there, too, even though it 
> hasn't been used before.
> 
Ok.  I put it there for rapid prototyping, but I also though it didn't really 
belong there.



Comment at: unittests/clangd/XRefsTests.cpp:271
+
+  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
+  MockFSProvider FS;

ilya-biryukov wrote:
> Specifying `/*UseRelPath=*/true` is not necessary for this test, default 
> value should do.
Ok.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 134306.
simark added a comment.

Generate Hover by pretty-printing the AST

This new version of the patch generates the hover by pretty-printing the AST.
The unit tests are updated accordingly.  There are some inconsistencies in how
things are printed.  For example, I find it a bit annoying that we print empty
curly braces after class names (e.g. "class Foo {}").  Also, namespace and
enums are printed with a newline between the two braces.  These are things that
could get fixed by doing changes to the clang AST printer.

The hover.test test now uses a more readable format.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -257,6 +257,311 @@
SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef input;
+StringRef expectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: unittests/clangd/XRefsTests.cpp:561
+
+EXPECT_EQ(H.contents.value, Test.expectedHover.str()) << Test.input;
+  }

Note that I used `.str()` here to make the output of failing tests readable and 
useful.  By default, gtest tries to print StringRef as if it was a container.  
I tried to make add a `PrintTo` function to specify how it should be printed, 
but couldn't get it to work (to have it called by the compiler), so I settled 
for this.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/XRefs.cpp:354
+
+  return Name;
+}

ilya-biryukov wrote:
> We should call `flush()` before returning `Name` here. The 
> `raw_string_ostream` is buffered.
I replaced it with `Stream.str()`.



Comment at: clangd/XRefs.cpp:373
+
+  return {};
+}

ilya-biryukov wrote:
> NIT: use `llvm::None` here instead of `{}`
Done.



Comment at: clangd/XRefs.cpp:394
+
+  //  SourceRange SR = D->getSourceRange();
+

ilya-biryukov wrote:
> Accidental leftover from previous code?
Oops, thanks.



Comment at: unittests/clangd/XRefsTests.cpp:262
+  struct OneTest {
+StringRef input;
+StringRef expectedHover;

ilya-biryukov wrote:
> NIT: LLVM uses `UpperCamelCase` for field names.
Ok, I fixed all the structs this patch adds.



Comment at: unittests/clangd/XRefsTests.cpp:561
+
+EXPECT_EQ(H.contents.value, Test.expectedHover.str()) << Test.input;
+  }

ilya-biryukov wrote:
> simark wrote:
> > Note that I used `.str()` here to make the output of failing tests readable 
> > and useful.  By default, gtest tries to print StringRef as if it was a 
> > container.  I tried to make add a `PrintTo` function to specify how it 
> > should be printed, but couldn't get it to work (to have it called by the 
> > compiler), so I settled for this.
> Thanks for spotting that.
> We have a fix for that in LLVM's gtest extensions (D43330).
> `str()` can now be removed.
Removed.  Even if this patch is merged before D43330 it's not a big deal, since 
it only affects debug output of failing test cases.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 134437.
simark added a comment.

New version

- Rebase on master, change findHover to have a callback-based interface
- Fix nits from previous review comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -259,6 +259,311 @@
SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },
+  {
+  R"cpp(// Typedef
+typede

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D35894#1008593, @ilya-biryukov wrote:

> Just a few last remarks and this is good to go.
>  Should I land it for you after the last comments are fixed?


I just received my commit access, so if you are ok with it, I would try to push 
it once I get your final OK.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D35894#1008861, @ilya-biryukov wrote:

> Only the naming of fields left.
>
> Also, please make sure to run `git-clang-format` on the code before 
> submitting to make sure it's formatted properly.


Ahh, I was running clang-format by hand, didn't know about git-clang-format 
(and obviously forgot some files). Thanks.




Comment at: clangd/ClangdLSPServer.cpp:331
+ if (!H) {
+   replyError(ErrorCode::InternalError,
+  llvm::toString(H.takeError()));

ilya-biryukov wrote:
> NIT: use `return replyError` to be consistent with other methods.
Ok.



Comment at: clangd/XRefs.cpp:354
+
+  return Name;
+}

ilya-biryukov wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > We should call `flush()` before returning `Name` here. The 
> > > `raw_string_ostream` is buffered.
> > I replaced it with `Stream.str()`.
> Also works, albeit less efficient (will probably do a copy where move is 
> enough).
Ah, didn't think about that.  I'll change it to flush + return Name.



Comment at: unittests/clangd/XRefsTests.cpp:262
+  struct OneTest {
+StringRef input;
+StringRef expectedHover;

ilya-biryukov wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > NIT: LLVM uses `UpperCamelCase` for field names.
> > Ok, I fixed all the structs this patch adds.
> Argh, sorry that I didn't mention it before, but we have an exception for 
> structs in `Procotol.h`, they follow the spelling in the LSP spec (i.e. 
> should be `lowerCamelCase`).
> 
Haha, no problem, changing them back is one keystroke away.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 134440.
simark added a comment.

Fix some nits

- Revert case change of struct fields in Protocol.h
- Change return formatting in onHover
- Use flush() + return Name in NamedDeclQualifiedName and TypeDeclToString


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -259,6 +259,311 @@
SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },
+ 

[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 134452.
simark added a comment.

Fix field names in XRefsTests.cpp

I realized I forgot to add some fixups in the previous version, the field names
in XRefsTests.cpp were not updated and therefore it did not build.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -259,6 +259,311 @@
SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },

[PATCH] D43454: [clangd] Do not reuse preamble if compile args changed

[Simon Marchi via Phabricator via cfe-commits]

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

I don't have the setup to test at the moment, but the code looks good to me.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43454



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D39571#1011877, @ilya-biryukov wrote:

> > That looks like another preamble-handling bug. It's much easier to fix and 
> > it's clangd-specific, so I'll make sure to fix that soon.
> >  Thanks for bringing this up, we haven't been testing compile command 
> > changes thoroughly. I'll come up with a fix shortly.
> > 
> > Before this is fixed, you could try writing a similar test with other, 
> > non-preprocessor, flags. See `ClangdVFSTest.ForceReparseCompileCommand` 
> > that tests a similar thing by changing `-xc` to `-xc++`.
> >  I'll make sure to test the `-D` case when fixing the bug.
>
> Should be fixed by https://reviews.llvm.org/D43454.


Thanks, I think I'll simply wait for that fix to get in.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 135153.
simark added a comment.

New version, take 2

The previous version contains the changes of already merged patches, I'm not
sure what I did wrong.  This is another try.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,6 +7,7 @@
 //
 //===--===//
 
+#include "Annotations.h"
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Matchers.h"
@@ -77,6 +78,39 @@
   VFSTag LastVFSTag = VFSTag();
 };
 
+// For each file, record whether the last published diagnostics contained at
+// least one error.
+class MultipleErrorCHeckingDiagConsumer : public DiagnosticsConsumer {
+public:
+  void
+  onDiagnosticsReady(PathRef File,
+ Tagged> Diagnostics) override {
+bool HadError = diagsContainErrors(Diagnostics.Value);
+
+std::lock_guard Lock(Mutex);
+LastDiagsHadError[File] = HadError;
+  }
+
+  bool contains(PathRef P) {
+std::lock_guard Lock(Mutex);
+return LastDiagsHadError.find(P) != LastDiagsHadError.end();
+  }
+
+  bool lastHadError(PathRef P) {
+std::lock_guard Lock(Mutex);
+return LastDiagsHadError[P];
+  }
+
+  void clear() {
+std::lock_guard Lock(Mutex);
+LastDiagsHadError.clear();
+  }
+
+private:
+  std::mutex Mutex;
+  std::map LastDiagsHadError;
+};
+
 /// Replaces all patterns of the form 0x123abc with spaces
 std::string replacePtrsInDump(std::string const &Dump) {
   llvm::Regex RE("0x[0-9a-fA-F]+");
@@ -413,6 +447,78 @@
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 }
 
+// Test ClangdServer.reparseOpenedFiles.
+TEST_F(ClangdVFSTest, ReparseOpenedFiles) {
+  Annotations FooSource(R"cpp(
+#ifdef MACRO
+$one[[static void bob() {}]]
+#else
+$two[[static void bob() {}]]
+#endif
+
+int main () { bo^b (); return 0; }
+)cpp");
+
+  Annotations BarSource(R"cpp(
+#ifdef MACRO
+this is an error
+#endif
+)cpp");
+
+  Annotations BazSource(R"cpp(
+int hello;
+)cpp");
+
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  MultipleErrorCHeckingDiagConsumer DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS,
+  /*AsyncThreadsCount=*/0,
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = testPath("foo.cpp");
+  auto BarCpp = testPath("bar.cpp");
+  auto BazCpp = testPath("baz.cpp");
+
+  FS.Files[FooCpp] = "";
+  FS.Files[BarCpp] = "";
+  FS.Files[BazCpp] = "";
+
+  CDB.ExtraClangFlags = {"-DMACRO=1"};
+  Server.addDocument(FooCpp, FooSource.code());
+  Server.addDocument(BarCpp, BarSource.code());
+  Server.addDocument(BazCpp, BazSource.code());
+
+  EXPECT_TRUE(DiagConsumer.contains(FooCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BarCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BazCpp));
+  EXPECT_FALSE(DiagConsumer.lastHadError(FooCpp));
+  EXPECT_TRUE(DiagConsumer.lastHadError(BarCpp));
+  EXPECT_FALSE(DiagConsumer.lastHadError(BazCpp));
+
+  auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("one")}));
+
+  // Undefine MACRO, close baz.cpp.
+  CDB.ExtraClangFlags.clear();
+  DiagConsumer.clear();
+  Server.removeDocument(BazCpp);
+  Server.reparseOpenedFiles();
+
+  EXPECT_TRUE(DiagConsumer.contains(FooCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BarCpp));
+  EXPECT_FALSE(DiagConsumer.contains(BazCpp));
+  EXPECT_FALSE(DiagConsumer.lastHadError(FooCpp));
+  EXPECT_FALSE(DiagConsumer.lastHadError(BarCpp));
+
+  Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("two")}));
+}
+
 TEST_F(ClangdVFSTest, MemoryUsage) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -52,6 +52,7 @@
   virtual void onRename(RenameParams &Parames) = 0;
   virtual void onDocumentHighlight(TextDocumentPositionParams &Params) = 0;
   virtual void onHover(TextDocumentPositionParams &Params) = 0;
+  virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark marked 5 inline comments as done.
simark added inline comments.



Comment at: unittests/clangd/ClangdTests.cpp:492
+
+  EXPECT_TRUE(DiagConsumer.contains(FooCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BarCpp));

ilya-biryukov wrote:
> Maybe expose a copy of the map from DiagConsumer and check for all files in a 
> single line?
> 
> ```
> class MultipleErrorCHeckingDiagConsumer {
>/// Exposes all files consumed by onDiagnosticsReady in an unspecified 
> order.
>/// For each file, a bool value indicates whether the last diagnostics 
> contained an error.
>std::vector> filesWithDiags() const { /* ... */ }
> };
> 
> /// 
> EXPECT_THAT(DiagConsumer.filesWithDiags(), UnorderedElementsAre(Pair(FooCpp, 
> false), Pair(BarCpp, true), Pair(BazCpp, false));
> ```
> 
> It would make the test more concise.
Nice :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark marked an inline comment as done.
simark added inline comments.



Comment at: unittests/clangd/ClangdTests.cpp:492
+
+  EXPECT_TRUE(DiagConsumer.contains(FooCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BarCpp));

simark wrote:
> ilya-biryukov wrote:
> > Maybe expose a copy of the map from DiagConsumer and check for all files in 
> > a single line?
> > 
> > ```
> > class MultipleErrorCHeckingDiagConsumer {
> >/// Exposes all files consumed by onDiagnosticsReady in an unspecified 
> > order.
> >/// For each file, a bool value indicates whether the last diagnostics 
> > contained an error.
> >std::vector> filesWithDiags() const { /* ... */ }
> > };
> > 
> > /// 
> > EXPECT_THAT(DiagConsumer.filesWithDiags(), 
> > UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true), Pair(BazCpp, 
> > false));
> > ```
> > 
> > It would make the test more concise.
> Nice :)
It's also better because we don't have to explicitly check that Baz is not 
there.  So if some other file sneaks in the result for some reason and 
shouldn't be there, the test will now fail, where it wouldn't have previously.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 135229.
simark added a comment.

New version

Address comments in https://reviews.llvm.org/D39571#1014237


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,6 +7,7 @@
 //
 //===--===//
 
+#include "Annotations.h"
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Matchers.h"
@@ -77,6 +78,43 @@
   VFSTag LastVFSTag = VFSTag();
 };
 
+/// For each file, record whether the last published diagnostics contained at
+/// least one error.
+class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer {
+public:
+  void
+  onDiagnosticsReady(PathRef File,
+ Tagged> Diagnostics) override {
+bool HadError = diagsContainErrors(Diagnostics.Value);
+
+std::lock_guard Lock(Mutex);
+LastDiagsHadError[File] = HadError;
+  }
+
+  /// Exposes all files consumed by onDiagnosticsReady in an unspecified order.
+  /// For each file, a bool value indicates whether the last diagnostics
+  /// contained an error.
+  std::vector> filesWithDiags() const {
+std::vector> Result;
+std::lock_guard Lock(Mutex);
+
+for (const auto &it : LastDiagsHadError) {
+  Result.emplace_back(it.first(), it.second);
+}
+
+return Result;
+  }
+
+  void clear() {
+std::lock_guard Lock(Mutex);
+LastDiagsHadError.clear();
+  }
+
+private:
+  mutable std::mutex Mutex;
+  llvm::StringMap LastDiagsHadError;
+};
+
 /// Replaces all patterns of the form 0x123abc with spaces
 std::string replacePtrsInDump(std::string const &Dump) {
   llvm::Regex RE("0x[0-9a-fA-F]+");
@@ -413,6 +451,72 @@
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 }
 
+// Test ClangdServer.reparseOpenedFiles.
+TEST_F(ClangdVFSTest, ReparseOpenedFiles) {
+  Annotations FooSource(R"cpp(
+#ifdef MACRO
+$one[[static void bob() {}]]
+#else
+$two[[static void bob() {}]]
+#endif
+
+int main () { bo^b (); return 0; }
+)cpp");
+
+  Annotations BarSource(R"cpp(
+#ifdef MACRO
+this is an error
+#endif
+)cpp");
+
+  Annotations BazSource(R"cpp(
+int hello;
+)cpp");
+
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  MultipleErrorCheckingDiagConsumer DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS,
+  /*AsyncThreadsCount=*/0,
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = testPath("foo.cpp");
+  auto BarCpp = testPath("bar.cpp");
+  auto BazCpp = testPath("baz.cpp");
+
+  FS.Files[FooCpp] = "";
+  FS.Files[BarCpp] = "";
+  FS.Files[BazCpp] = "";
+
+  CDB.ExtraClangFlags = {"-DMACRO=1"};
+  Server.addDocument(FooCpp, FooSource.code());
+  Server.addDocument(BarCpp, BarSource.code());
+  Server.addDocument(BazCpp, BazSource.code());
+
+  EXPECT_THAT(DiagConsumer.filesWithDiags(),
+  UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true),
+   Pair(BazCpp, false)));
+
+  auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("one")}));
+
+  // Undefine MACRO, close baz.cpp.
+  CDB.ExtraClangFlags.clear();
+  DiagConsumer.clear();
+  Server.removeDocument(BazCpp);
+  Server.reparseOpenedFiles();
+
+  EXPECT_THAT(DiagConsumer.filesWithDiags(),
+  UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, false)));
+
+  Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("two")}));
+}
+
 TEST_F(ClangdVFSTest, MemoryUsage) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -52,6 +52,7 @@
   virtual void onRename(RenameParams &Parames) = 0;
   virtual void onDocumentHighlight(TextDocumentPositionParams &Params) = 0;
   virtual void onHover(TextDocumentPositionParams &Params) = 0;
+  virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
Index: clangd/ProtocolHandl

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Simon Marchi via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL325784: [clangd] DidChangeConfiguration Notification 
(authored by simark, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D39571?vs=135229&id=135407#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D39571

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/clangd/DraftStore.cpp
  clang-tools-extra/trunk/clangd/DraftStore.h
  clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
  clang-tools-extra/trunk/clangd/ProtocolHandlers.h
  clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
@@ -7,6 +7,7 @@
 //
 //===--===//
 
+#include "Annotations.h"
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Matchers.h"
@@ -77,6 +78,43 @@
   VFSTag LastVFSTag = VFSTag();
 };
 
+/// For each file, record whether the last published diagnostics contained at
+/// least one error.
+class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer {
+public:
+  void
+  onDiagnosticsReady(PathRef File,
+ Tagged> Diagnostics) override {
+bool HadError = diagsContainErrors(Diagnostics.Value);
+
+std::lock_guard Lock(Mutex);
+LastDiagsHadError[File] = HadError;
+  }
+
+  /// Exposes all files consumed by onDiagnosticsReady in an unspecified order.
+  /// For each file, a bool value indicates whether the last diagnostics
+  /// contained an error.
+  std::vector> filesWithDiags() const {
+std::vector> Result;
+std::lock_guard Lock(Mutex);
+
+for (const auto &it : LastDiagsHadError) {
+  Result.emplace_back(it.first(), it.second);
+}
+
+return Result;
+  }
+
+  void clear() {
+std::lock_guard Lock(Mutex);
+LastDiagsHadError.clear();
+  }
+
+private:
+  mutable std::mutex Mutex;
+  llvm::StringMap LastDiagsHadError;
+};
+
 /// Replaces all patterns of the form 0x123abc with spaces
 std::string replacePtrsInDump(std::string const &Dump) {
   llvm::Regex RE("0x[0-9a-fA-F]+");
@@ -413,6 +451,72 @@
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 }
 
+// Test ClangdServer.reparseOpenedFiles.
+TEST_F(ClangdVFSTest, ReparseOpenedFiles) {
+  Annotations FooSource(R"cpp(
+#ifdef MACRO
+$one[[static void bob() {}]]
+#else
+$two[[static void bob() {}]]
+#endif
+
+int main () { bo^b (); return 0; }
+)cpp");
+
+  Annotations BarSource(R"cpp(
+#ifdef MACRO
+this is an error
+#endif
+)cpp");
+
+  Annotations BazSource(R"cpp(
+int hello;
+)cpp");
+
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  MultipleErrorCheckingDiagConsumer DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS,
+  /*AsyncThreadsCount=*/0,
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = testPath("foo.cpp");
+  auto BarCpp = testPath("bar.cpp");
+  auto BazCpp = testPath("baz.cpp");
+
+  FS.Files[FooCpp] = "";
+  FS.Files[BarCpp] = "";
+  FS.Files[BazCpp] = "";
+
+  CDB.ExtraClangFlags = {"-DMACRO=1"};
+  Server.addDocument(FooCpp, FooSource.code());
+  Server.addDocument(BarCpp, BarSource.code());
+  Server.addDocument(BazCpp, BazSource.code());
+
+  EXPECT_THAT(DiagConsumer.filesWithDiags(),
+  UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true),
+   Pair(BazCpp, false)));
+
+  auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("one")}));
+
+  // Undefine MACRO, close baz.cpp.
+  CDB.ExtraClangFlags.clear();
+  DiagConsumer.clear();
+  Server.removeDocument(BazCpp);
+  Server.reparseOpenedFiles();
+
+  EXPECT_THAT(DiagConsumer.filesWithDiags(),
+  UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, false)));
+
+  Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+ FooSource.range("two")}));
+}
+
 TEST_F(ClangdVFSTest, MemoryUsage) {
   MockFSProvid

[PATCH] D44213: [clangd] Remove unused field in HandlerRegisterer

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 137410.
simark added a comment.

Fix formatting


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44213

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h


Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -55,7 +55,7 @@
   virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
 };
 
-void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
+void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
   ProtocolCallbacks &Callbacks);
 
 } // namespace clangd
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -38,16 +38,14 @@
   }
 
   JSONRPCDispatcher &Dispatcher;
-  JSONOutput *Out;
   ProtocolCallbacks *Callbacks;
 };
 
 } // namespace
 
 void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
-  JSONOutput &Out,
   ProtocolCallbacks &Callbacks) {
-  HandlerRegisterer Register{Dispatcher, &Out, &Callbacks};
+  HandlerRegisterer Register{Dispatcher, &Callbacks};
 
   Register("initialize", &ProtocolCallbacks::onInitialize);
   Register("shutdown", &ProtocolCallbacks::onShutdown);
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -409,7 +409,7 @@
   JSONRPCDispatcher Dispatcher([](const json::Expr &Params) {
 replyError(ErrorCode::MethodNotFound, "method not found");
   });
-  registerCallbackHandlers(Dispatcher, Out, /*Callbacks=*/*this);
+  registerCallbackHandlers(Dispatcher, /*Callbacks=*/*this);
 
   // Run the Language Server loop.
   runLanguageServerLoop(In, Out, InputStyle, Dispatcher, IsDone);


Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -55,7 +55,7 @@
   virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
 };
 
-void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
+void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
   ProtocolCallbacks &Callbacks);
 
 } // namespace clangd
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -38,16 +38,14 @@
   }
 
   JSONRPCDispatcher &Dispatcher;
-  JSONOutput *Out;
   ProtocolCallbacks *Callbacks;
 };
 
 } // namespace
 
 void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
-  JSONOutput &Out,
   ProtocolCallbacks &Callbacks) {
-  HandlerRegisterer Register{Dispatcher, &Out, &Callbacks};
+  HandlerRegisterer Register{Dispatcher, &Callbacks};
 
   Register("initialize", &ProtocolCallbacks::onInitialize);
   Register("shutdown", &ProtocolCallbacks::onShutdown);
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -409,7 +409,7 @@
   JSONRPCDispatcher Dispatcher([](const json::Expr &Params) {
 replyError(ErrorCode::MethodNotFound, "method not found");
   });
-  registerCallbackHandlers(Dispatcher, Out, /*Callbacks=*/*this);
+  registerCallbackHandlers(Dispatcher, /*Callbacks=*/*this);
 
   // Run the Language Server loop.
   runLanguageServerLoop(In, Out, InputStyle, Dispatcher, IsDone);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44213: [clangd] Remove unused field in HandlerRegisterer

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, ilya-biryukov, 
klimek.

Tested by rebuilding.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44213

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h


Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -55,8 +55,7 @@
   virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
 };
 
-void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
-  ProtocolCallbacks &Callbacks);
+void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, ProtocolCallbacks 
&Callbacks);
 
 } // namespace clangd
 } // namespace clang
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -38,16 +38,14 @@
   }
 
   JSONRPCDispatcher &Dispatcher;
-  JSONOutput *Out;
   ProtocolCallbacks *Callbacks;
 };
 
 } // namespace
 
 void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
-  JSONOutput &Out,
   ProtocolCallbacks &Callbacks) {
-  HandlerRegisterer Register{Dispatcher, &Out, &Callbacks};
+  HandlerRegisterer Register{Dispatcher, &Callbacks};
 
   Register("initialize", &ProtocolCallbacks::onInitialize);
   Register("shutdown", &ProtocolCallbacks::onShutdown);
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -409,7 +409,7 @@
   JSONRPCDispatcher Dispatcher([](const json::Expr &Params) {
 replyError(ErrorCode::MethodNotFound, "method not found");
   });
-  registerCallbackHandlers(Dispatcher, Out, /*Callbacks=*/*this);
+  registerCallbackHandlers(Dispatcher, /*Callbacks=*/*this);
 
   // Run the Language Server loop.
   runLanguageServerLoop(In, Out, InputStyle, Dispatcher, IsDone);


Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -55,8 +55,7 @@
   virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
 };
 
-void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
-  ProtocolCallbacks &Callbacks);
+void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, ProtocolCallbacks &Callbacks);
 
 } // namespace clangd
 } // namespace clang
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -38,16 +38,14 @@
   }
 
   JSONRPCDispatcher &Dispatcher;
-  JSONOutput *Out;
   ProtocolCallbacks *Callbacks;
 };
 
 } // namespace
 
 void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
-  JSONOutput &Out,
   ProtocolCallbacks &Callbacks) {
-  HandlerRegisterer Register{Dispatcher, &Out, &Callbacks};
+  HandlerRegisterer Register{Dispatcher, &Callbacks};
 
   Register("initialize", &ProtocolCallbacks::onInitialize);
   Register("shutdown", &ProtocolCallbacks::onShutdown);
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -409,7 +409,7 @@
   JSONRPCDispatcher Dispatcher([](const json::Expr &Params) {
 replyError(ErrorCode::MethodNotFound, "method not found");
   });
-  registerCallbackHandlers(Dispatcher, Out, /*Callbacks=*/*this);
+  registerCallbackHandlers(Dispatcher, /*Callbacks=*/*this);
 
   // Run the Language Server loop.
   runLanguageServerLoop(In, Out, InputStyle, Dispatcher, IsDone);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44213: [clangd] Remove unused field in HandlerRegisterer

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE326947: [clangd] Remove unused field in HandlerRegisterer 
(authored by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D44213?vs=137410&id=137471#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44213

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h


Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -38,16 +38,14 @@
   }
 
   JSONRPCDispatcher &Dispatcher;
-  JSONOutput *Out;
   ProtocolCallbacks *Callbacks;
 };
 
 } // namespace
 
 void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
-  JSONOutput &Out,
   ProtocolCallbacks &Callbacks) {
-  HandlerRegisterer Register{Dispatcher, &Out, &Callbacks};
+  HandlerRegisterer Register{Dispatcher, &Callbacks};
 
   Register("initialize", &ProtocolCallbacks::onInitialize);
   Register("shutdown", &ProtocolCallbacks::onShutdown);
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -55,7 +55,7 @@
   virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
 };
 
-void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
+void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
   ProtocolCallbacks &Callbacks);
 
 } // namespace clangd
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -409,7 +409,7 @@
   JSONRPCDispatcher Dispatcher([](const json::Expr &Params) {
 replyError(ErrorCode::MethodNotFound, "method not found");
   });
-  registerCallbackHandlers(Dispatcher, Out, /*Callbacks=*/*this);
+  registerCallbackHandlers(Dispatcher, /*Callbacks=*/*this);
 
   // Run the Language Server loop.
   runLanguageServerLoop(In, Out, InputStyle, Dispatcher, IsDone);


Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -38,16 +38,14 @@
   }
 
   JSONRPCDispatcher &Dispatcher;
-  JSONOutput *Out;
   ProtocolCallbacks *Callbacks;
 };
 
 } // namespace
 
 void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
-  JSONOutput &Out,
   ProtocolCallbacks &Callbacks) {
-  HandlerRegisterer Register{Dispatcher, &Out, &Callbacks};
+  HandlerRegisterer Register{Dispatcher, &Callbacks};
 
   Register("initialize", &ProtocolCallbacks::onInitialize);
   Register("shutdown", &ProtocolCallbacks::onShutdown);
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -55,7 +55,7 @@
   virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
 };
 
-void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
+void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
   ProtocolCallbacks &Callbacks);
 
 } // namespace clangd
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -409,7 +409,7 @@
   JSONRPCDispatcher Dispatcher([](const json::Expr &Params) {
 replyError(ErrorCode::MethodNotFound, "method not found");
   });
-  registerCallbackHandlers(Dispatcher, Out, /*Callbacks=*/*this);
+  registerCallbackHandlers(Dispatcher, /*Callbacks=*/*this);
 
   // Run the Language Server loop.
   runLanguageServerLoop(In, Out, InputStyle, Dispatcher, IsDone);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44226: [clangd] Add -log-to-stderr option

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, ioeric, JDevlieghere, jkorous-apple, 
ilya-biryukov, klimek.

Currently, clangd prints all the LSP communication on stderr.  While
this is good for developping, I don't think it's good as a general
default.  For example, we are integrating clangd (and other language
servers) in an IDE, and we print everything the language servers send on
their stderr on our own stderr.  Printing the whole LSP communication
adds a lot of noise.  If there are actual error messages to be printed,
then it makes sense to output them on stderr.

Also, an IDE or tool that integrates many language servers will likely
have its own way of logging LSP communication in a consistent way, so
it's better to leave it to that tool to log the LSP communication if it
wants to.

An alternative would be to introduce log levels (debug, info, warning,
error), and output LSP communications only at the debug log level.

Signed-off-by: Simon Marchi 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226

Files:
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/tool/ClangdMain.cpp
  test/clangd/protocol.test
  test/clangd/too_large.test

Index: test/clangd/too_large.test
===
--- test/clangd/too_large.test
+++ test/clangd/too_large.test
@@ -1,4 +1,4 @@
-# RUN: not clangd -run-synchronously < %s 2>&1 | FileCheck -check-prefix=STDERR %s
+# RUN: not clangd -run-synchronously -log-to-stderr < %s 2>&1 | FileCheck -check-prefix=STDERR %s
 # vim: fileformat=dos
 # It is absolutely vital that this file has CRLF line endings.
 #
Index: test/clangd/protocol.test
===
--- test/clangd/protocol.test
+++ test/clangd/protocol.test
@@ -1,5 +1,5 @@
-# RUN: not clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s
-# RUN: not clangd -pretty -run-synchronously < %s 2>&1 | FileCheck -check-prefix=STDERR %s
+# RUN: not clangd -pretty -log-to-stderr -run-synchronously < %s | FileCheck -strict-whitespace %s
+# RUN: not clangd -pretty -log-to-stderr -run-synchronously < %s 2>&1 | FileCheck -check-prefix=STDERR %s
 # vim: fileformat=dos
 # It is absolutely vital that this file has CRLF line endings.
 #
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -77,15 +77,20 @@
 llvm::cl::init(JSONStreamStyle::Standard));
 
 static llvm::cl::opt
+LogToStderr("log-to-stderr",
+llvm::cl::desc("Output LSP communication on stderr"),
+llvm::cl::init(false));
+
+static llvm::cl::opt
 PrettyPrint("pretty", llvm::cl::desc("Pretty-print JSON output"),
 llvm::cl::init(false));
 
-static llvm::cl::opt Test(
-"lit-test",
-llvm::cl::desc(
-"Abbreviation for -input-style=delimited -pretty -run-synchronously. "
-"Intended to simplify lit tests."),
-llvm::cl::init(false), llvm::cl::Hidden);
+static llvm::cl::opt
+Test("lit-test",
+ llvm::cl::desc(
+ "Abbreviation for -input-style=delimited -pretty -log-to-stderr "
+ "-run-synchronously. Intended to simplify lit tests."),
+ llvm::cl::init(false), llvm::cl::Hidden);
 
 static llvm::cl::opt PCHStorage(
 "pch-storage",
@@ -140,6 +145,7 @@
   if (Test) {
 RunSynchronously = true;
 InputStyle = JSONStreamStyle::Delimited;
+LogToStderr = true;
 PrettyPrint = true;
   }
 
@@ -190,7 +196,7 @@
 
   JSONOutput Out(llvm::outs(), llvm::errs(),
  InputMirrorStream ? InputMirrorStream.getPointer() : nullptr,
- PrettyPrint);
+ LogToStderr, PrettyPrint);
 
   clangd::LoggingSession LoggingSession(Out);
 
Index: clangd/JSONRPCDispatcher.h
===
--- clangd/JSONRPCDispatcher.h
+++ clangd/JSONRPCDispatcher.h
@@ -30,8 +30,10 @@
   // JSONOutput now that we pass Context everywhere.
 public:
   JSONOutput(llvm::raw_ostream &Outs, llvm::raw_ostream &Logs,
- llvm::raw_ostream *InputMirror = nullptr, bool Pretty = false)
-  : Pretty(Pretty), Outs(Outs), Logs(Logs), InputMirror(InputMirror) {}
+ llvm::raw_ostream *InputMirror = nullptr, bool LogToStderr = false,
+ bool Pretty = false)
+  : LogToStderr(LogToStderr), Pretty(Pretty), Outs(Outs), Logs(Logs),
+InputMirror(InputMirror) {}
 
   /// Emit a JSONRPC message.
   void writeMessage(const json::Expr &Result);
@@ -44,6 +46,9 @@
   /// Unlike other methods of JSONOutput, mirrorInput is not thread-safe.
   void mirrorInput(const Twine &Message);
 
+  // Whether to output all LSP communication to stderr.
+  const bool LogToStderr;
+
   // Whether output should be pretty-printed.
   const bool Pretty;
 
Index: clangd/JSONRPCDispatcher.cpp
==

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 137478.
simark added a comment.

Changed -log-to-stderr to -log-lsp-to-stderr

The first version disabled a bit too much, this version removes the LSP
communication logging in a more fine grained way.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/tool/ClangdMain.cpp

Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -77,6 +77,11 @@
 llvm::cl::init(JSONStreamStyle::Standard));
 
 static llvm::cl::opt
+LogLspToStderr("log-lsp-to-stderr",
+   llvm::cl::desc("Output LSP communication on stderr"),
+   llvm::cl::init(false));
+
+static llvm::cl::opt
 PrettyPrint("pretty", llvm::cl::desc("Pretty-print JSON output"),
 llvm::cl::init(false));
 
@@ -190,7 +195,7 @@
 
   JSONOutput Out(llvm::outs(), llvm::errs(),
  InputMirrorStream ? InputMirrorStream.getPointer() : nullptr,
- PrettyPrint);
+ LogLspToStderr, PrettyPrint);
 
   clangd::LoggingSession LoggingSession(Out);
 
@@ -238,5 +243,7 @@
   llvm::set_thread_name("clangd.main");
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
-  return LSPServer.run(std::cin, InputStyle) ? 0 : NoShutdownRequestErrorCode;
+  return LSPServer.run(std::cin, InputStyle, LogLspToStderr)
+ ? 0
+ : NoShutdownRequestErrorCode;
 }
Index: clangd/JSONRPCDispatcher.h
===
--- clangd/JSONRPCDispatcher.h
+++ clangd/JSONRPCDispatcher.h
@@ -30,8 +30,10 @@
   // JSONOutput now that we pass Context everywhere.
 public:
   JSONOutput(llvm::raw_ostream &Outs, llvm::raw_ostream &Logs,
- llvm::raw_ostream *InputMirror = nullptr, bool Pretty = false)
-  : Pretty(Pretty), Outs(Outs), Logs(Logs), InputMirror(InputMirror) {}
+ llvm::raw_ostream *InputMirror = nullptr,
+ bool LogLspToStderr = false, bool Pretty = false)
+  : LogLspToStderr(LogLspToStderr), Pretty(Pretty), Outs(Outs), Logs(Logs),
+InputMirror(InputMirror) {}
 
   /// Emit a JSONRPC message.
   void writeMessage(const json::Expr &Result);
@@ -44,6 +46,9 @@
   /// Unlike other methods of JSONOutput, mirrorInput is not thread-safe.
   void mirrorInput(const Twine &Message);
 
+  /// Whether to output all LSP communication to stderr.
+  const bool LogLspToStderr;
+
   // Whether output should be pretty-printed.
   const bool Pretty;
 
@@ -104,7 +109,8 @@
 /// replacements of \r\n with \n.
 void runLanguageServerLoop(std::istream &In, JSONOutput &Out,
JSONStreamStyle InputStyle,
-   JSONRPCDispatcher &Dispatcher, bool &IsDone);
+   JSONRPCDispatcher &Dispatcher, bool LogLspToStderr,
+   bool &IsDone);
 
 } // namespace clangd
 } // namespace clang
Index: clangd/JSONRPCDispatcher.cpp
===
--- clangd/JSONRPCDispatcher.cpp
+++ clangd/JSONRPCDispatcher.cpp
@@ -66,7 +66,9 @@
 Outs << "Content-Length: " << S.size() << "\r\n\r\n" << S;
 Outs.flush();
   }
-  log(llvm::Twine("--> ") + S);
+
+  if (LogLspToStderr)
+log(llvm::Twine("--> ") + S);
 }
 
 void JSONOutput::log(const Twine &Message) {
@@ -299,14 +301,17 @@
 void clangd::runLanguageServerLoop(std::istream &In, JSONOutput &Out,
JSONStreamStyle InputStyle,
JSONRPCDispatcher &Dispatcher,
-   bool &IsDone) {
+   bool LogLspToStderr, bool &IsDone) {
   auto &ReadMessage =
   (InputStyle == Delimited) ? readDelimitedMessage : readStandardMessage;
   while (In.good()) {
 if (auto JSON = ReadMessage(In, Out)) {
   if (auto Doc = json::parse(*JSON)) {
-// Log the formatted message.
-log(llvm::formatv(Out.Pretty ? "<-- {0:2}\n" : "<-- {0}\n", *Doc));
+if (LogLspToStderr) {
+  // Log the formatted message.
+  log(llvm::formatv(Out.Pretty ? "<-- {0:2}\n" : "<-- {0}\n", *Doc));
+}
+
 // Finally, execute the action for this JSON message.
 if (!Dispatcher.call(*Doc, Out))
   log("JSON dispatch failed!\n");
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -42,7 +42,8 @@
   ///
   /// \return Wether we received a 'shutdown' request before an 'exit' request
   bool run(std::istream &In,
-   JSONStreamStyle InputStyle = JSONStreamStyle::Standard);
+   JSONStreamStyle InputStyle = JSONStreamStyle::Standard,

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D44226#1030625, @ilya-biryukov wrote:

> I would vouch for adding a log level instead. It's a very well understood 
> concept that certainly covers this use-case and can be useful in other places.
>  WDYT?


I agree.  How would you prefer the flags to look like?

- --log-level {debug,info,warning,error} (warning being the default if not 
specified)
- Use -v multiple times to be more verbose: "-v" to show info, "-v -v" to show 
debug.  Without -v, we show warnings and errors.
- something else?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226



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

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 137573.
simark added a comment.

Update

- Change switch to -verbose
- Add vlog function, do the filtering there


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226

Files:
  clangd/JSONRPCDispatcher.cpp
  clangd/Logger.cpp
  clangd/Logger.h
  clangd/tool/ClangdMain.cpp

Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -76,6 +76,9 @@
"messages delimited by --- lines, with # comment support")),
 llvm::cl::init(JSONStreamStyle::Standard));
 
+static llvm::cl::opt Verbose("verbose", llvm::cl::desc("Be more verbose"),
+   llvm::cl::init(false));
+
 static llvm::cl::opt
 PrettyPrint("pretty", llvm::cl::desc("Pretty-print JSON output"),
 llvm::cl::init(false));
@@ -192,7 +195,7 @@
  InputMirrorStream ? InputMirrorStream.getPointer() : nullptr,
  PrettyPrint);
 
-  clangd::LoggingSession LoggingSession(Out);
+  clangd::LoggingSession LoggingSession(Out, Verbose);
 
   // If --compile-commands-dir arg was invoked, check value and override default
   // path.
Index: clangd/Logger.h
===
--- clangd/Logger.h
+++ clangd/Logger.h
@@ -15,11 +15,14 @@
 namespace clang {
 namespace clangd {
 
-/// Main logging function.
+/// Main logging functions.
 /// Logs messages to a global logger, which can be set up by LoggingSesssion.
 /// If no logger is registered, writes to llvm::errs().
 void log(const llvm::Twine &Message);
 
+/// Same as the above, but for verbose messages.
+void vlog(const llvm::Twine &Message);
+
 /// Interface to allow custom logging in clangd.
 class Logger {
 public:
@@ -32,7 +35,7 @@
 /// Only one LoggingSession can be active at a time.
 class LoggingSession {
 public:
-  LoggingSession(clangd::Logger &Instance);
+  LoggingSession(clangd::Logger &Instance, bool Verbose);
   ~LoggingSession();
 
   LoggingSession(LoggingSession &&) = delete;
Index: clangd/Logger.cpp
===
--- clangd/Logger.cpp
+++ clangd/Logger.cpp
@@ -16,11 +16,13 @@
 
 namespace {
 Logger *L = nullptr;
+bool Verbose_ = false;
 } // namespace
 
-LoggingSession::LoggingSession(clangd::Logger &Instance) {
+LoggingSession::LoggingSession(clangd::Logger &Instance, bool Verbose) {
   assert(!L);
   L = &Instance;
+  Verbose_ = Verbose;
 }
 
 LoggingSession::~LoggingSession() { L = nullptr; }
@@ -35,5 +37,10 @@
   }
 }
 
+void vlog(const llvm::Twine &Message) {
+  if (Verbose_)
+log(Message);
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/JSONRPCDispatcher.cpp
===
--- clangd/JSONRPCDispatcher.cpp
+++ clangd/JSONRPCDispatcher.cpp
@@ -66,7 +66,7 @@
 Outs << "Content-Length: " << S.size() << "\r\n\r\n" << S;
 Outs.flush();
   }
-  log(llvm::Twine("--> ") + S);
+  vlog(llvm::Twine("--> ") + S);
 }
 
 void JSONOutput::log(const Twine &Message) {
@@ -306,7 +306,7 @@
 if (auto JSON = ReadMessage(In, Out)) {
   if (auto Doc = json::parse(*JSON)) {
 // Log the formatted message.
-log(llvm::formatv(Out.Pretty ? "<-- {0:2}\n" : "<-- {0}\n", *Doc));
+vlog(llvm::formatv(Out.Pretty ? "<-- {0:2}\n" : "<-- {0}\n", *Doc));
 // Finally, execute the action for this JSON message.
 if (!Dispatcher.call(*Doc, Out))
   log("JSON dispatch failed!\n");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Now, if the client calls a method that we do not support (), clangd just 
outputs:

  C/C++: [10:55:16.033] Error -32601: method not found

It would be useful to at least print in this error message the name of the 
method.  Because the "unknown method" handler shares the type signature of 
regular handlers, I would have to add a parameter with the method name to all 
existing handlers.  I'll try that.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226



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

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

[Simon Marchi via Phabricator via cfe-commits]

simark marked an inline comment as done.
simark added inline comments.



Comment at: clangd/Logger.cpp:19
 Logger *L = nullptr;
+bool Verbose_ = false;
 } // namespace

ilya-biryukov wrote:
> Could we move the flag to implementation of `Logger`?
> I.e.:
> ```
> class Logger {
>   virtual log(const llvm::Twine &Message, bool Verbose);
> };
> 
> // Implementation of top-level log
> void clangd::log(const llvm::Twine &Message) {
>L->log(Message, /*Verbose=*/false);
>// should also handle missing Logger by logging into llvm::errs()
> }
> 
> // Implementation of top-level vlog.
> void clangd::vlog(const llvm::Twine &Message) {
>L->log(Message, /*Verbose=*/true);
>// should also handle missing Logger by logging into llvm::errs()
> }
> ```
> 
> An implementation of the interface would decide whether to log or not, based 
> on command-line argument.
That's what I thought of doing first.  The issue is that if we don't set a 
logger with LoggingSession, the log function prints to stderr itself.  So if we 
don't check the Verbose flag there, the -v flag will have no effect when 
LoggingSession has not been called (I don't know if it's really a problem or 
not, since in practice we call it).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226



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

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/tool/ClangdMain.cpp:79
 
+static llvm::cl::opt Verbose("verbose", llvm::cl::desc("Be more 
verbose"),
+   llvm::cl::init(false));

ilya-biryukov wrote:
> Maybe just call it `-v`?
I would have like to add both "-v" and "-verbose", but it doesn't seem possible 
to have two flags for the same option.  "-v" it is then, it is quite standard.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226



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

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 137582.
simark added a comment.

Update

- Add vlog method to Logger interface
- Add method name to "method not found" error message


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226

Files:
  clangd/ClangdLSPServer.cpp
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Logger.cpp
  clangd/Logger.h
  clangd/ProtocolHandlers.cpp
  clangd/tool/ClangdMain.cpp
  test/clangd/unsupported-method.test

Index: test/clangd/unsupported-method.test
===
--- test/clangd/unsupported-method.test
+++ test/clangd/unsupported-method.test
@@ -6,7 +6,7 @@
 {"jsonrpc":"2.0","id":1,"method":"textDocument/jumpInTheAirLikeYouJustDontCare","params":{}}
 #  CHECK:  "error": {
 # CHECK-NEXT:"code": -32601,
-# CHECK-NEXT:"message": "method not found"
+# CHECK-NEXT:"message": "method not found (textDocument/jumpInTheAirLikeYouJustDontCare)"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 1,
 # CHECK-NEXT:  "jsonrpc": "2.0"
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -76,6 +76,9 @@
"messages delimited by --- lines, with # comment support")),
 llvm::cl::init(JSONStreamStyle::Standard));
 
+static llvm::cl::opt Verbose("v", llvm::cl::desc("Be more verbose"),
+   llvm::cl::init(false));
+
 static llvm::cl::opt
 PrettyPrint("pretty", llvm::cl::desc("Pretty-print JSON output"),
 llvm::cl::init(false));
@@ -190,9 +193,9 @@
 
   JSONOutput Out(llvm::outs(), llvm::errs(),
  InputMirrorStream ? InputMirrorStream.getPointer() : nullptr,
- PrettyPrint);
+ PrettyPrint, Verbose);
 
-  clangd::LoggingSession LoggingSession(Out);
+  clangd::LoggingSession LoggingSession(Out, Verbose);
 
   // If --compile-commands-dir arg was invoked, check value and override default
   // path.
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -27,14 +27,15 @@
   void operator()(StringRef Method, void (ProtocolCallbacks::*Handler)(Param)) {
 // Capture pointers by value, as the lambda will outlive this object.
 auto *Callbacks = this->Callbacks;
-Dispatcher.registerHandler(Method, [=](const json::Expr &RawParams) {
-  typename std::remove_reference::type P;
-  if (fromJSON(RawParams, P)) {
-(Callbacks->*Handler)(P);
-  } else {
-log("Failed to decode " + Method + " request.");
-  }
-});
+Dispatcher.registerHandler(
+Method, [=](StringRef Method, const json::Expr &RawParams) {
+  typename std::remove_reference::type P;
+  if (fromJSON(RawParams, P)) {
+(Callbacks->*Handler)(P);
+  } else {
+log("Failed to decode " + Method + " request.");
+  }
+});
   }
 
   JSONRPCDispatcher &Dispatcher;
Index: clangd/Logger.h
===
--- clangd/Logger.h
+++ clangd/Logger.h
@@ -15,24 +15,28 @@
 namespace clang {
 namespace clangd {
 
-/// Main logging function.
+/// Main logging functions.
 /// Logs messages to a global logger, which can be set up by LoggingSesssion.
 /// If no logger is registered, writes to llvm::errs().
 void log(const llvm::Twine &Message);
 
+/// Same as the above, but for verbose messages.
+void vlog(const llvm::Twine &Message);
+
 /// Interface to allow custom logging in clangd.
 class Logger {
 public:
   virtual ~Logger() = default;
 
-  /// Implementations of this method must be thread-safe.
+  /// Implementations of these methods must be thread-safe.
   virtual void log(const llvm::Twine &Message) = 0;
+  virtual void vlog(const llvm::Twine &Message) = 0;
 };
 
 /// Only one LoggingSession can be active at a time.
 class LoggingSession {
 public:
-  LoggingSession(clangd::Logger &Instance);
+  LoggingSession(clangd::Logger &Instance, bool Verbose);
   ~LoggingSession();
 
   LoggingSession(LoggingSession &&) = delete;
Index: clangd/Logger.cpp
===
--- clangd/Logger.cpp
+++ clangd/Logger.cpp
@@ -16,11 +16,14 @@
 
 namespace {
 Logger *L = nullptr;
+bool Verbose_ = false;
+
 } // namespace
 
-LoggingSession::LoggingSession(clangd::Logger &Instance) {
+LoggingSession::LoggingSession(clangd::Logger &Instance, bool Verbose) {
   assert(!L);
   L = &Instance;
+  Verbose_ = Verbose;
 }
 
 LoggingSession::~LoggingSession() { L = nullptr; }
@@ -35,5 +38,12 @@
   }
 }
 
+void vlog(const llvm::Twine &Message) {
+  if (L)
+L->vlog(Message);
+  else
+log(Message);
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/JSONRPCDispatcher.h
==

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

[Simon Marchi via Phabricator via cfe-commits]

simark marked 2 inline comments as done.
simark added inline comments.



Comment at: clangd/tool/ClangdMain.cpp:79
 
+static llvm::cl::opt Verbose("verbose", llvm::cl::desc("Be more 
verbose"),
+   llvm::cl::init(false));

ilya-biryukov wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > Maybe just call it `-v`?
> > I would have like to add both "-v" and "-verbose", but it doesn't seem 
> > possible to have two flags for the same option.  "-v" it is then, it is 
> > quite standard.
> I would go with having just `-v` with no aliases.
> 
> But this should do the trick if you prefer to have `-verbose` as an option:
> ```
> llvm::cl::opt Verbose("v", llvm::cl::alias("verbose") , //
> ```
Ah ok thanks for the info.  I'll leave it with just `-v` here, but knowing this 
could be handy in the future.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226



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

[PATCH] D44272: [clangd] Support incremental document syncing

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, ilya-biryukov, 
mgorny, klimek.
simark added a reviewer: malaperle.

This patch adds support for incremental document syncing, as described
in the LSP spec.  The protocol specifies ranges in terms of Position (a
line and a character), and our drafts are stored as plain strings.  So I
see two things that may not be super efficient for very large files:

- Converting a Position to an offset (the positionToOffset function) requires 
searching for end of lines until we reach the desired line.
- When we update a range, we construct a new string, which implies copying the 
whole document.

However, for the typical size of a C++ document and the frequency of
update (at which a user types), it may not be an issue.  This patch aims
at getting the basic feature in, and we can always improve it later if
we find it's too slow.

Signed-off-by: Simon Marchi 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/CMakeLists.txt
  unittests/clangd/DraftStoreTests.cpp

Index: unittests/clangd/DraftStoreTests.cpp
===
--- /dev/null
+++ unittests/clangd/DraftStoreTests.cpp
@@ -0,0 +1,164 @@
+//===-- DraftStoreTests.cpp - Clangd unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Annotations.h"
+#include "DraftStore.h"
+#include "Protocol.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+using namespace llvm;
+
+namespace {
+using testing::ElementsAre;
+using testing::IsEmpty;
+
+struct IncrementalTestStep {
+  StringRef Src;
+  StringRef Contents;
+};
+
+static void
+runIncrementalUpdateTest(llvm::ArrayRef Steps) {
+  DraftStore DS;
+  std::string NewContents;
+  Annotations InitialSrc(Steps[0].Src);
+  const char Path[] = "/hello.cpp";
+
+  // Set the initial content.
+  DS.updateDraft(Path, InitialSrc.code(), {}, &NewContents);
+  EXPECT_EQ(NewContents, InitialSrc.code());
+  EXPECT_EQ(*DS.getDraft(Path).Draft, InitialSrc.code());
+
+  // For each step, replace the range marked in \p Src with \p Contents, and
+  // verify that it's equal to the following step's \p Src.
+  for (size_t i = 1; i < Steps.size(); i++) {
+Annotations SrcBefore(Steps[i - 1].Src);
+Annotations SrcAfter(Steps[i].Src);
+StringRef Contents = Steps[i - 1].Contents;
+
+DS.updateDraft(Path, Contents, SrcBefore.range(), &NewContents);
+EXPECT_EQ(NewContents, SrcAfter.code());
+EXPECT_EQ(*DS.getDraft(Path).Draft, SrcAfter.code());
+  }
+}
+
+TEST(DraftStoreIncrementalUpdateTest, Simple) {
+  // clang-format off
+  IncrementalTestStep Steps[] =
+{
+  // Replace a range
+  {
+R"cpp(static int
+hello[[World]]()
+{})cpp",
+"Universe"
+  },
+  // Delete a range
+  {
+R"cpp(static int
+hello[[Universe]]()
+{})cpp",
+""
+  },
+  // Add a range
+  {
+R"cpp(static int
+hello[[]]()
+{})cpp",
+"Monde"
+  },
+  {
+R"cpp(static int
+helloMonde()
+{})cpp",
+""
+  }
+};
+  // clang-format on
+
+  runIncrementalUpdateTest(Steps);
+}
+
+TEST(DraftStoreIncrementalUpdateTest, MultiLine) {
+  // clang-format off
+  IncrementalTestStep Steps[] =
+{
+  // Replace a range
+  {
+R"cpp(static [[int
+helloWorld]]()
+{})cpp",
+R"cpp(char
+welcome)cpp"
+  },
+  // Delete a range
+  {
+R"cpp(static char[[
+welcome]]()
+{})cpp",
+""
+  },
+  // Add a range
+  {
+R"cpp(static char[[]]()
+{})cpp",
+R"cpp(
+cookies)cpp"
+  },
+  {
+R"cpp(static char
+cookies()
+{})cpp",
+""
+  }
+};
+  // clang-format on
+
+  runIncrementalUpdateTest(Steps);
+}
+
+TEST(DraftStoreUpdateTest, Simple) {
+  StringRef Src =
+  R"cpp(static int
+helloWorld()
+{})cpp";
+
+  DraftStore DS;
+  StringRef Path = "/hello.cpp";
+  std::string NewContents;
+
+  // Add a document
+  DS.updateDraft(Path, Src, {}, &NewContents);
+  EXPECT_EQ(NewContents, Src);
+  EXPECT_EQ(*DS.getDraft(Path).Draft, Src);
+  EXPECT_THAT(DS.getActiveFiles(), ElementsAre(Path));
+
+  StringRef Src2 =
+  R"cpp(static char
+thisIsAFunction()
+{})cpp";
+  DS.updateDraft(Path, Src2, {}, &NewContents);
+  EXPECT_EQ(NewContents, Src2);
+  EXPECT_EQ(*DS.getDraft(Path).Draft, Src2);
+  EXPECT_THAT(DS.getActiveFiles(), ElementsAre(Path));
+
+  DS.removeDraft(Path);
+  EXPECT_FALSE(DS.getDraft(Path).Draft.ha

[PATCH] D44272: [clangd] Support incremental document syncing

[Simon Marchi via Phabricator via cfe-commits]

simark marked 2 inline comments as done.
simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:101
 json::obj{
-{"textDocumentSync", 1},
+{"textDocumentSync", 2},
 {"documentFormattingProvider", true},

ilya-biryukov wrote:
> A comment mentioning that 2 means incremental document sync from LSP would be 
> useful here.
I opted to add a TextDocumentSyncKind enum to Protocol.h and use that.  Though 
instead of an enum, I did a class with static const fields:

```
struct TextDocumentSyncKind {
  /// Documents should not be synced at all.
  static const int None = 0;

  /// Documents are synced by always sending the full content of the document.
  static const int Full = 1;

  /// Documents are synced by sending the full content on open.  After that
  /// only incremental updates to the document are send.
  static const int Incremental = 2;
};
```

otherwise it would've required an (int) cast when using it to generate JSON:

{"textDocumentSync", TextDocumentSyncKind::Incremental},



Comment at: clangd/ClangdLSPServer.cpp:149
 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) 
{
   if (Params.contentChanges.size() != 1)
 return replyError(ErrorCode::InvalidParams,

ilya-biryukov wrote:
> We should handle more than a single change here.
Ok, I'll try that.  I am not sure I interpret the spec correctly.  If you have 
two changes, the second change applies to the state of the document after 
having applied the first change, is that right?  If that's the case, I think we 
only need to iterate on the changes and call addDocument on them sequentially 
(which could be done in the DraftMgr, given the interface change to DraftStore 
you suggest lower).



Comment at: clangd/ClangdServer.cpp:557
 // 64-bit unsigned integer.
 if (Version < LastReportedDiagsVersion)
   return;

ilya-biryukov wrote:
> When you'll try remove the `DraftMgr`, this piece of code will be hard to 
> refactor because it relies on `DocVersion`.
> This is a workaround for a possible race condition that should really be 
> handled by TUScheduler rather than this code, but we haven't got around to 
> fixing it yet. (It's on the list, though, would probably get in next week).
> 
> A proper workaround for now would be to add `llvm::StringMap 
> InternalVersions` field to `ClangdServer`, increment those versions on each 
> call to `addDocument` and use them here in the same way we currently use 
> DraftMgr's versions.
Hmm ok, it will probably make sense when I try to do it.  The 
`InternalVersions` map maps URIs to the latest version seen?

Is the race condition because we could get diagnostics for a stale version of 
the document, so we don't want to emit them?



Comment at: clangd/ClangdServer.h:159
   /// constructor will receive onDiagnosticsReady callback.
   void addDocument(PathRef File, StringRef Contents,
+   WantDiagnostics WantDiags = WantDiagnostics::Auto,

ilya-biryukov wrote:
> I don't think we should do this on `ClangdServer` level. We will have to copy 
> the whole file anyway before passing it to clang, so there are no performance 
> wins here and it complicates the interface.
> I suggest we update the document text from diffs on `ClangdLSPServer` level 
> and continue passing the whole document to `ClangdServer`.
> It would mean `DraftMgr` will need to move to `ClangdLSPServer`, but it's 
> fine.
> 
`ClangdServer` also needs `DraftMgr` for `forceReparse` and 
`reparseOpenedFiles`.  I guess that too would move to `ClangdLSPServer`?  I'm 
just not sure how to adapt the unit tests, since we don't have unittests using 
`ClangdLSPServer` (yet).



Comment at: clangd/DraftStore.cpp:47
+DocVersion DraftStore::updateDraft(PathRef File, StringRef Contents,
+   llvm::Optional range,
+   std::string *NewContents) {

ilya-biryukov wrote:
> NIT: LLVM uses `UpperCamelCase` for parameters and local variables
Woops.  I should learn to use clang-tidy.  It found other instances (the local 
variables) but it doesn't find the parameters not written in camel case.  Do 
you have an idea why?  I dumped the config and see these:

```
  - key: readability-identifier-naming.ClassCase
value:   CamelCase
  - key: readability-identifier-naming.EnumCase
value:   CamelCase
  - key: readability-identifier-naming.UnionCase
value:   CamelCase
  - key: readability-identifier-naming.VariableCase
value:   CamelCase
```

I assume there must be a `ParamCase` or something like that, but I can't find 
the exhaustive list of parameters for that check.



Comment at: clangd/DraftStore.cpp:66
+
+newContents = Entry.Draft-

[PATCH] D44272: [clangd] Support incremental document syncing

[Simon Marchi via Phabricator via cfe-commits]

simark marked 2 inline comments as done.
simark added inline comments.



Comment at: clangd/ClangdServer.h:159
   /// constructor will receive onDiagnosticsReady callback.
   void addDocument(PathRef File, StringRef Contents,
+   WantDiagnostics WantDiags = WantDiagnostics::Auto,

simark wrote:
> ilya-biryukov wrote:
> > I don't think we should do this on `ClangdServer` level. We will have to 
> > copy the whole file anyway before passing it to clang, so there are no 
> > performance wins here and it complicates the interface.
> > I suggest we update the document text from diffs on `ClangdLSPServer` level 
> > and continue passing the whole document to `ClangdServer`.
> > It would mean `DraftMgr` will need to move to `ClangdLSPServer`, but it's 
> > fine.
> > 
> `ClangdServer` also needs `DraftMgr` for `forceReparse` and 
> `reparseOpenedFiles`.  I guess that too would move to `ClangdLSPServer`?  I'm 
> just not sure how to adapt the unit tests, since we don't have unittests 
> using `ClangdLSPServer` (yet).
Actually, `forceReparse` doesn't really need `DraftMgr` (it's only used for the 
assert), only `reparseOpenedFiles` needs it.  So in the test, we can manually 
call `forceReparse` on all the files by hand... this just means that 
`reparseOpenedFiles` won't be tested anymore.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272



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

[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, ilya-biryukov, 
mgorny, klimek.

This patch moves the draft manager closer to the edge of Clangd, from
ClangdServer to ClangdLSPServer.  This will make it easier to implement
incremental document sync, by making ClangdServer only deal with
complete documents.

As a result, DraftStore doesn't have to deal with versioning, and thus
its API can be simplified.  It is replaced by a StringMap in
ClangdServer holding a current version number for each file.

The current implementation of reparseOpenedFiles is racy.  It calls
getActiveDrafts and calls forceReparse for each of them.  forceReparse
gets the draft fromt he DraftStore.  In theory, it's possible for a file
to have been closed in the mean time.  I replaced getActiveDrafts by
forEachActiveDraft.  It has the advantage that the DraftStore lock is
held while we iterate on the drafts.

Signed-off-by: Simon Marchi 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44408

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/DraftStoreTests.cpp
  unittests/clangd/SyncAPI.cpp
  unittests/clangd/SyncAPI.h

Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -22,11 +22,13 @@
 void runAddDocument(ClangdServer &Server, PathRef File, StringRef Contents);
 
 Tagged runCodeComplete(ClangdServer &Server, PathRef File,
-   Position Pos,
+   StringRef Contents, Position Pos,
clangd::CodeCompleteOptions Opts);
 
-llvm::Expected>
-runSignatureHelp(ClangdServer &Server, PathRef File, Position Pos);
+llvm::Expected> runSignatureHelp(ClangdServer &Server,
+   PathRef File,
+   StringRef Contents,
+   Position Pos);
 
 llvm::Expected>>
 runFindDefinitions(ClangdServer &Server, PathRef File, Position Pos);
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -68,17 +68,19 @@
 } // namespace
 
 Tagged runCodeComplete(ClangdServer &Server, PathRef File,
-   Position Pos,
+   StringRef Contents, Position Pos,
clangd::CodeCompleteOptions Opts) {
   llvm::Optional> Result;
-  Server.codeComplete(File, Pos, Opts, capture(Result));
+  Server.codeComplete(File, Contents, Pos, Opts, capture(Result));
   return std::move(*Result);
 }
 
-llvm::Expected>
-runSignatureHelp(ClangdServer &Server, PathRef File, Position Pos) {
+llvm::Expected> runSignatureHelp(ClangdServer &Server,
+   PathRef File,
+   StringRef Contents,
+   Position Pos) {
   llvm::Optional>> Result;
-  Server.signatureHelp(File, Pos, capture(Result));
+  Server.signatureHelp(File, Contents, Pos, capture(Result));
   return std::move(*Result);
 }
 
Index: unittests/clangd/DraftStoreTests.cpp
===
--- /dev/null
+++ unittests/clangd/DraftStoreTests.cpp
@@ -0,0 +1,56 @@
+//===-- DraftStoreTests.cpp - Clangd unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "DraftStore.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using namespace llvm;
+
+using ::testing::UnorderedElementsAre;
+
+/// Get the active drafts from \p DS as a vector.
+static std::vector getActiveDrafts(const DraftStore &DS) {
+  std::vector ActiveDrafts;
+
+  DS.forEachActiveDraft([&ActiveDrafts](PathRef File, StringRef Contents) {
+ActiveDrafts.push_back(File);
+  });
+
+  return ActiveDrafts;
+}
+
+TEST(DraftStoreTest, forEachActiveDraft) {
+  DraftStore DS;
+
+  DS.updateDraft("/foo.cpp", "Foo");
+  DS.updateDraft("/bar.cpp", "Bar");
+  DS.updateDraft("/baz.cpp", "Baz");
+
+  std::vector Drafts = getActiveDrafts(DS);
+  EXPECT_THAT(Drafts, UnorderedElementsAre("/foo.cpp", "/bar.cpp", "/baz.cpp"));
+
+  DS.removeDraft("/bar.cpp");
+
+  Drafts = getActiveDrafts(DS);
+  EXPECT_T

[PATCH] D44272: [clangd] Support incremental document syncing

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

I uploaded a new patch that moves the draft store to ClangdLSPServer, that 
implements what you suggested.

https://reviews.llvm.org/D44408

I will update the incremental sync patch once that patch is in.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272



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

[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 138253.
simark added a comment.

Rebase

Non-trivial rebase on today's master.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44408

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/DraftStoreTests.cpp
  unittests/clangd/SyncAPI.cpp
  unittests/clangd/SyncAPI.h

Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -22,11 +22,13 @@
 void runAddDocument(ClangdServer &Server, PathRef File, StringRef Contents);
 
 llvm::Expected
-runCodeComplete(ClangdServer &Server, PathRef File, Position Pos,
-clangd::CodeCompleteOptions Opts);
+runCodeComplete(ClangdServer &Server, PathRef File, std::string Contents,
+Position Pos, clangd::CodeCompleteOptions Opts);
 
 llvm::Expected runSignatureHelp(ClangdServer &Server,
-   PathRef File, Position Pos);
+   PathRef File,
+   std::string Contents,
+   Position Pos);
 
 llvm::Expected>
 runFindDefinitions(ClangdServer &Server, PathRef File, Position Pos);
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -68,17 +68,19 @@
 } // namespace
 
 llvm::Expected
-runCodeComplete(ClangdServer &Server, PathRef File, Position Pos,
-clangd::CodeCompleteOptions Opts) {
+runCodeComplete(ClangdServer &Server, PathRef File, std::string Contents,
+Position Pos, clangd::CodeCompleteOptions Opts) {
   llvm::Optional> Result;
-  Server.codeComplete(File, Pos, Opts, capture(Result));
+  Server.codeComplete(File, std::move(Contents), Pos, Opts, capture(Result));
   return std::move(*Result);
 }
 
 llvm::Expected runSignatureHelp(ClangdServer &Server,
-   PathRef File, Position Pos) {
+   PathRef File,
+   std::string Contents,
+   Position Pos) {
   llvm::Optional> Result;
-  Server.signatureHelp(File, Pos, capture(Result));
+  Server.signatureHelp(File, std::move(Contents), Pos, capture(Result));
   return std::move(*Result);
 }
 
Index: unittests/clangd/DraftStoreTests.cpp
===
--- /dev/null
+++ unittests/clangd/DraftStoreTests.cpp
@@ -0,0 +1,56 @@
+//===-- DraftStoreTests.cpp - Clangd unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "DraftStore.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using namespace llvm;
+
+using ::testing::UnorderedElementsAre;
+
+/// Get the active drafts from \p DS as a vector.
+static std::vector getActiveDrafts(const DraftStore &DS) {
+  std::vector ActiveDrafts;
+
+  DS.forEachActiveDraft([&ActiveDrafts](PathRef File, StringRef Contents) {
+ActiveDrafts.push_back(File);
+  });
+
+  return ActiveDrafts;
+}
+
+TEST(DraftStoreTest, forEachActiveDraft) {
+  DraftStore DS;
+
+  DS.updateDraft("/foo.cpp", "Foo");
+  DS.updateDraft("/bar.cpp", "Bar");
+  DS.updateDraft("/baz.cpp", "Baz");
+
+  std::vector Drafts = getActiveDrafts(DS);
+  EXPECT_THAT(Drafts, UnorderedElementsAre("/foo.cpp", "/bar.cpp", "/baz.cpp"));
+
+  DS.removeDraft("/bar.cpp");
+
+  Drafts = getActiveDrafts(DS);
+  EXPECT_THAT(Drafts, UnorderedElementsAre("/foo.cpp", "/baz.cpp"));
+
+  DS.removeDraft("/another.cpp");
+
+  Drafts = getActiveDrafts(DS);
+  EXPECT_THAT(Drafts, UnorderedElementsAre("/foo.cpp", "/baz.cpp"));
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -122,7 +122,7 @@
   Annotations Test(Text);
   runAddDocument(Server, File, Test.code());
   auto CompletionList =
-  cantFail(runCodeComplete(Server, File, Test.point(), Opts));
+  cantFail(runCodeComplete(Server, File, Test.code(), Test.point(), Opts));
   // Sanity-check that filterText is valid.
   EXPECT_THAT(CompletionList.items, Each(NameContainsFilter()));
   return CompletionL

[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D44408#1036846, @ilya-biryukov wrote:

> We shouldn't add `Contents` parameter to every method, it would defeat the 
> purpose of caching ASTs and won't allow to properly manage lifetimes of the 
> indexes.


Makes sense.

> The most tricky part is getting rid of `DraftMgr` in `forceReparse`. Here's a 
> change that removes usages of it there: https://reviews.llvm.org/D44462`, it 
> should be much easier to make it work when it lands.

I'll take a look, thanks!

> Any other reason why we need to add them?

No, just ignorance on my part.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44408



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

[PATCH] D44462: [clangd] Don't use DraftMgr in implementation of forceReparse.

[Simon Marchi via Phabricator via cfe-commits]

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

I rebased my patch (https://reviews.llvm.org/D44408) on top of this one, it 
looks good.




Comment at: clangd/ClangdServer.cpp:143
+  tooling::CompileCommand NewCommand = CompileArgs.getCompileCommand(File);
+  DocVersion Version = DraftMgr.getVersion(File);
+  Path FileStr = File.str();

I was wondering if we should increment the version here.  From what I 
understand, it is used to identify each version of the parse, so that when 
diagnostics are ready, we know if they are for the latest version (and should 
be sent to the user) or if they are stale and should be ignored.  If we don't 
change the document content but change the compile commands, I would consider 
it as a new version, since the diagnostics coming from the same document but 
old compile commands should be considered stale.



Comment at: clangd/TUScheduler.h:62
 
+  /// Returns a list of currently tracked files. File starts being trakced on
+  /// first update() call to it and stops being tracked on remove() call.

"trakced"


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44462



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

[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 138344.
simark added a comment.

Rebase on https://reviews.llvm.org/D44462


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44408

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/SyncAPI.cpp
  unittests/clangd/SyncAPI.h

Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -22,11 +22,13 @@
 void runAddDocument(ClangdServer &Server, PathRef File, StringRef Contents);
 
 llvm::Expected
-runCodeComplete(ClangdServer &Server, PathRef File, Position Pos,
-clangd::CodeCompleteOptions Opts);
+runCodeComplete(ClangdServer &Server, PathRef File, std::string Contents,
+Position Pos, clangd::CodeCompleteOptions Opts);
 
 llvm::Expected runSignatureHelp(ClangdServer &Server,
-   PathRef File, Position Pos);
+   PathRef File,
+   std::string Contents,
+   Position Pos);
 
 llvm::Expected>
 runFindDefinitions(ClangdServer &Server, PathRef File, Position Pos);
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -68,17 +68,19 @@
 } // namespace
 
 llvm::Expected
-runCodeComplete(ClangdServer &Server, PathRef File, Position Pos,
-clangd::CodeCompleteOptions Opts) {
+runCodeComplete(ClangdServer &Server, PathRef File, std::string Contents,
+Position Pos, clangd::CodeCompleteOptions Opts) {
   llvm::Optional> Result;
-  Server.codeComplete(File, Pos, Opts, capture(Result));
+  Server.codeComplete(File, std::move(Contents), Pos, Opts, capture(Result));
   return std::move(*Result);
 }
 
 llvm::Expected runSignatureHelp(ClangdServer &Server,
-   PathRef File, Position Pos) {
+   PathRef File,
+   std::string Contents,
+   Position Pos) {
   llvm::Optional> Result;
-  Server.signatureHelp(File, Pos, capture(Result));
+  Server.signatureHelp(File, std::move(Contents), Pos, capture(Result));
   return std::move(*Result);
 }
 
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -122,7 +122,7 @@
   Annotations Test(Text);
   runAddDocument(Server, File, Test.code());
   auto CompletionList =
-  cantFail(runCodeComplete(Server, File, Test.point(), Opts));
+  cantFail(runCodeComplete(Server, File, Test.code(), Test.point(), Opts));
   // Sanity-check that filterText is valid.
   EXPECT_THAT(CompletionList.items, Each(NameContainsFilter()));
   return CompletionList;
@@ -537,16 +537,17 @@
 
   auto I = memIndex({var("ns::index")});
   Opts.Index = I.get();
-  auto WithIndex = cantFail(runCodeComplete(Server, File, Test.point(), Opts));
+  auto WithIndex =
+  cantFail(runCodeComplete(Server, File, Test.code(), Test.point(), Opts));
   EXPECT_THAT(WithIndex.items,
   UnorderedElementsAre(Named("local"), Named("index")));
-  auto ClassFromPreamble =
-  cantFail(runCodeComplete(Server, File, Test.point("2"), Opts));
+  auto ClassFromPreamble = cantFail(
+  runCodeComplete(Server, File, Test.code(), Test.point("2"), Opts));
   EXPECT_THAT(ClassFromPreamble.items, Contains(Named("member")));
 
   Opts.Index = nullptr;
   auto WithoutIndex =
-  cantFail(runCodeComplete(Server, File, Test.point(), Opts));
+  cantFail(runCodeComplete(Server, File, Test.code(), Test.point(), Opts));
   EXPECT_THAT(WithoutIndex.items,
   UnorderedElementsAre(Named("local"), Named("preamble")));
 }
@@ -577,7 +578,8 @@
   )cpp");
   runAddDocument(Server, File, Test.code());
 
-  auto Results = cantFail(runCodeComplete(Server, File, Test.point(), {}));
+  auto Results =
+  cantFail(runCodeComplete(Server, File, Test.code(), Test.point(), {}));
   // "XYZ" and "foo" are not included in the file being completed but are still
   // visible through the index.
   EXPECT_THAT(Results.items, Has("XYZ", CompletionItemKind::Class));
@@ -616,7 +618,7 @@
   auto File = testPath("foo.cpp");
   Annotations Test(Text);
   runAddDocument(Server, File, Test.code());
-  return cantFail(runSignatureHelp(Server, File, Test.point()));
+  return cantFail(runSignatureHelp(Server, File, Test.code(), Test.point()));
 }
 
 MATCHER_P(ParamsAre, P, "") {
Index: unittests/clangd/ClangdTests.cpp

[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

I don't see how to avoid adding the Contents parameter to the codeComplete and 
signatureHelp methods, since they needed to fetch the document from the draft 
manager and pass it to the backend.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44408



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

[PATCH] D44462: [clangd] Don't use DraftMgr in implementation of forceReparse.

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/TUScheduler.h:69
+  /// FIXME: remove the callback from this function
+  void updateCompileCommand(PathRef File, tooling::CompileCommand NewCommand,
+IntrusiveRefCntPtr FS,

sammccall wrote:
> (summarizing offline discussion)
> 
> this is so close to `update`, it'd be nice if we could just call `update` 
> instead.
> 
> For that we need the contents, so forceReparse needs contents, so... can 
> forceReparse just be addDocument(skipCache=true) or something?
> 
> Requiring content to be passed doesn't seem like a big burden in practice, 
> and makes it clear that clangdserver is never responsible for maintaining 
> copies of the content on the callers behalf (and clangdlspserver is).
> 
> reparseOpenFiles needs to move to clangdlspserver, but this seems consistent 
> with the design. (so I think we can drop getTrackedFiles?)
I also thought it would be nice to have only one method `update`.  What about 
if the `Contents` member of `ParseInputs` is optional?  When it is not 
instantiated (such as when calling `forceReparse`), it would mean to re-use the 
previously sent source.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44462



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