[PATCH] D50375: [clangd] Share getSymbolID implementation.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ioeric.
Herald added subscribers: arphaman, jkorous, MaskRay, ilya-biryukov.

And remove all duplicated implementation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50375

Files:
  clangd/AST.cpp
  clangd/AST.h
  clangd/CodeComplete.cpp
  clangd/XRefs.cpp
  clangd/index/SymbolCollector.cpp

Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -320,21 +320,20 @@
   if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
 return true;
 
-  llvm::SmallString<128> USR;
-  if (index::generateUSRForDecl(ND, USR))
+  auto ID = getSymbolID(ND);
+  if (!ID)
 return true;
-  SymbolID ID(USR);
 
   const NamedDecl &OriginalDecl = *cast(ASTNode.OrigD);
-  const Symbol *BasicSymbol = Symbols.find(ID);
+  const Symbol *BasicSymbol = Symbols.find(*ID);
   if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
-BasicSymbol = addDeclaration(*ND, std::move(ID));
+BasicSymbol = addDeclaration(*ND, std::move(*ID));
   else if (isPreferredDeclaration(OriginalDecl, Roles))
 // If OriginalDecl is preferred, replace the existing canonical
 // declaration (e.g. a class forward declaration). There should be at most
 // one duplicate as we expect to see only one preferred declaration per
 // TU, because in practice they are definitions.
-BasicSymbol = addDeclaration(OriginalDecl, std::move(ID));
+BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID));
 
   if (Roles & static_cast(index::SymbolRole::Definition))
 addDefinition(OriginalDecl, *BasicSymbol);
@@ -423,9 +422,9 @@
 }
   };
   for (const NamedDecl *ND : ReferencedDecls) {
-llvm::SmallString<128> USR;
-if (!index::generateUSRForDecl(ND, USR))
-  IncRef(SymbolID(USR));
+if (auto ID = getSymbolID(ND)) {
+  IncRef(*ID);
+}
   }
   if (Opts.CollectMacro) {
 assert(PP);
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -202,15 +202,6 @@
   return L;
 }
 
-// Get the symbol ID for a declaration, if possible.
-llvm::Optional getSymbolID(const Decl *D) {
-  llvm::SmallString<128> USR;
-  if (index::generateUSRForDecl(D, USR)) {
-return None;
-  }
-  return SymbolID(USR);
-}
-
 } // namespace
 
 std::vector findDefinitions(ParsedAST &AST, Position Pos,
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -396,10 +396,7 @@
   switch (R.Kind) {
   case CodeCompletionResult::RK_Declaration:
   case CodeCompletionResult::RK_Pattern: {
-llvm::SmallString<128> USR;
-if (/*Ignore=*/clang::index::generateUSRForDecl(R.Declaration, USR))
-  return None;
-return SymbolID(USR);
+return clang::clangd::getSymbolID(R.Declaration);
   }
   case CodeCompletionResult::RK_Macro:
 // FIXME: Macros do have USRs, but the CCR doesn't contain enough info.
Index: clangd/AST.h
===
--- clangd/AST.h
+++ clangd/AST.h
@@ -16,6 +16,7 @@
 
 #include "clang/AST/Decl.h"
 #include "clang/Basic/SourceLocation.h"
+#include "index/Index.h"
 
 namespace clang {
 class SourceManager;
@@ -33,6 +34,10 @@
 /// like inline namespaces.
 std::string printQualifiedName(const NamedDecl &ND);
 
+/// Gets the symbol ID for a declaration.
+/// Returns None if fails.
+llvm::Optional getSymbolID(const Decl *D);
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/AST.cpp
===
--- clangd/AST.cpp
+++ clangd/AST.cpp
@@ -12,6 +12,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Index/USRGeneration.h"
 
 namespace clang {
 namespace clangd {
@@ -53,5 +54,13 @@
   return QName;
 }
 
+llvm::Optional getSymbolID(const Decl *D) {
+  llvm::SmallString<128> USR;
+  if (index::generateUSRForDecl(D, USR)) {
+return None;
+  }
+  return SymbolID(USR);
+}
+
 } // namespace clangd
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50375: [clangd] Share getSymbolID implementation.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/CodeComplete.cpp:399
   case CodeCompletionResult::RK_Pattern: {
-llvm::SmallString<128> USR;
-if (/*Ignore=*/clang::index::generateUSRForDecl(R.Declaration, USR))
-  return None;
-return SymbolID(USR);
+return clang::clangd::getSymbolID(R.Declaration);
   }

ioeric wrote:
> No need for namespace qualifiers?
We need the qualifiers to disambiguate the function name, because this function 
name is also `getSymbolID`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50375



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

[PATCH] D50375: [clangd] Share getSymbolID implementation.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 159466.
hokein marked 3 inline comments as done.
hokein added a comment.

Address comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50375

Files:
  clangd/AST.cpp
  clangd/AST.h
  clangd/CodeComplete.cpp
  clangd/XRefs.cpp
  clangd/index/SymbolCollector.cpp

Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -320,21 +320,20 @@
   if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
 return true;
 
-  llvm::SmallString<128> USR;
-  if (index::generateUSRForDecl(ND, USR))
+  auto ID = getSymbolID(ND);
+  if (!ID)
 return true;
-  SymbolID ID(USR);
 
   const NamedDecl &OriginalDecl = *cast(ASTNode.OrigD);
-  const Symbol *BasicSymbol = Symbols.find(ID);
+  const Symbol *BasicSymbol = Symbols.find(*ID);
   if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
-BasicSymbol = addDeclaration(*ND, std::move(ID));
+BasicSymbol = addDeclaration(*ND, std::move(*ID));
   else if (isPreferredDeclaration(OriginalDecl, Roles))
 // If OriginalDecl is preferred, replace the existing canonical
 // declaration (e.g. a class forward declaration). There should be at most
 // one duplicate as we expect to see only one preferred declaration per
 // TU, because in practice they are definitions.
-BasicSymbol = addDeclaration(OriginalDecl, std::move(ID));
+BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID));
 
   if (Roles & static_cast(index::SymbolRole::Definition))
 addDefinition(OriginalDecl, *BasicSymbol);
@@ -423,9 +422,9 @@
 }
   };
   for (const NamedDecl *ND : ReferencedDecls) {
-llvm::SmallString<128> USR;
-if (!index::generateUSRForDecl(ND, USR))
-  IncRef(SymbolID(USR));
+if (auto ID = getSymbolID(ND)) {
+  IncRef(*ID);
+}
   }
   if (Opts.CollectMacro) {
 assert(PP);
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -202,15 +202,6 @@
   return L;
 }
 
-// Get the symbol ID for a declaration, if possible.
-llvm::Optional getSymbolID(const Decl *D) {
-  llvm::SmallString<128> USR;
-  if (index::generateUSRForDecl(D, USR)) {
-return None;
-  }
-  return SymbolID(USR);
-}
-
 } // namespace
 
 std::vector findDefinitions(ParsedAST &AST, Position Pos,
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -396,10 +396,7 @@
   switch (R.Kind) {
   case CodeCompletionResult::RK_Declaration:
   case CodeCompletionResult::RK_Pattern: {
-llvm::SmallString<128> USR;
-if (/*Ignore=*/clang::index::generateUSRForDecl(R.Declaration, USR))
-  return None;
-return SymbolID(USR);
+return clang::clangd::getSymbolID(R.Declaration);
   }
   case CodeCompletionResult::RK_Macro:
 // FIXME: Macros do have USRs, but the CCR doesn't contain enough info.
Index: clangd/AST.h
===
--- clangd/AST.h
+++ clangd/AST.h
@@ -16,6 +16,7 @@
 
 #include "clang/AST/Decl.h"
 #include "clang/Basic/SourceLocation.h"
+#include "index/Index.h"
 
 namespace clang {
 class SourceManager;
@@ -33,6 +34,9 @@
 /// like inline namespaces.
 std::string printQualifiedName(const NamedDecl &ND);
 
+/// Gets the symbol ID for a declaration, if possible.
+llvm::Optional getSymbolID(const Decl *D);
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/AST.cpp
===
--- clangd/AST.cpp
+++ clangd/AST.cpp
@@ -12,6 +12,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Index/USRGeneration.h"
 
 namespace clang {
 namespace clangd {
@@ -53,5 +54,12 @@
   return QName;
 }
 
+llvm::Optional getSymbolID(const Decl *D) {
+  llvm::SmallString<128> USR;
+  if (index::generateUSRForDecl(D, USR))
+return None;
+  return SymbolID(USR);
+}
+
 } // namespace clangd
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50375: [clangd] Share getSymbolID implementation.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339116: [clangd] Share getSymbolID implementation. (authored 
by hokein, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D50375

Files:
  clang-tools-extra/trunk/clangd/AST.cpp
  clang-tools-extra/trunk/clangd/AST.h
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/XRefs.cpp
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp

Index: clang-tools-extra/trunk/clangd/AST.cpp
===
--- clang-tools-extra/trunk/clangd/AST.cpp
+++ clang-tools-extra/trunk/clangd/AST.cpp
@@ -12,6 +12,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Index/USRGeneration.h"
 
 namespace clang {
 namespace clangd {
@@ -53,5 +54,12 @@
   return QName;
 }
 
+llvm::Optional getSymbolID(const Decl *D) {
+  llvm::SmallString<128> USR;
+  if (index::generateUSRForDecl(D, USR))
+return None;
+  return SymbolID(USR);
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
@@ -320,21 +320,20 @@
   if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
 return true;
 
-  llvm::SmallString<128> USR;
-  if (index::generateUSRForDecl(ND, USR))
+  auto ID = getSymbolID(ND);
+  if (!ID)
 return true;
-  SymbolID ID(USR);
 
   const NamedDecl &OriginalDecl = *cast(ASTNode.OrigD);
-  const Symbol *BasicSymbol = Symbols.find(ID);
+  const Symbol *BasicSymbol = Symbols.find(*ID);
   if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
-BasicSymbol = addDeclaration(*ND, std::move(ID));
+BasicSymbol = addDeclaration(*ND, std::move(*ID));
   else if (isPreferredDeclaration(OriginalDecl, Roles))
 // If OriginalDecl is preferred, replace the existing canonical
 // declaration (e.g. a class forward declaration). There should be at most
 // one duplicate as we expect to see only one preferred declaration per
 // TU, because in practice they are definitions.
-BasicSymbol = addDeclaration(OriginalDecl, std::move(ID));
+BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID));
 
   if (Roles & static_cast(index::SymbolRole::Definition))
 addDefinition(OriginalDecl, *BasicSymbol);
@@ -423,9 +422,9 @@
 }
   };
   for (const NamedDecl *ND : ReferencedDecls) {
-llvm::SmallString<128> USR;
-if (!index::generateUSRForDecl(ND, USR))
-  IncRef(SymbolID(USR));
+if (auto ID = getSymbolID(ND)) {
+  IncRef(*ID);
+}
   }
   if (Opts.CollectMacro) {
 assert(PP);
Index: clang-tools-extra/trunk/clangd/AST.h
===
--- clang-tools-extra/trunk/clangd/AST.h
+++ clang-tools-extra/trunk/clangd/AST.h
@@ -16,6 +16,7 @@
 
 #include "clang/AST/Decl.h"
 #include "clang/Basic/SourceLocation.h"
+#include "index/Index.h"
 
 namespace clang {
 class SourceManager;
@@ -33,6 +34,9 @@
 /// like inline namespaces.
 std::string printQualifiedName(const NamedDecl &ND);
 
+/// Gets the symbol ID for a declaration, if possible.
+llvm::Optional getSymbolID(const Decl *D);
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/trunk/clangd/XRefs.cpp
===
--- clang-tools-extra/trunk/clangd/XRefs.cpp
+++ clang-tools-extra/trunk/clangd/XRefs.cpp
@@ -202,15 +202,6 @@
   return L;
 }
 
-// Get the symbol ID for a declaration, if possible.
-llvm::Optional getSymbolID(const Decl *D) {
-  llvm::SmallString<128> USR;
-  if (index::generateUSRForDecl(D, USR)) {
-return None;
-  }
-  return SymbolID(USR);
-}
-
 } // namespace
 
 std::vector findDefinitions(ParsedAST &AST, Position Pos,
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -396,10 +396,7 @@
   switch (R.Kind) {
   case CodeCompletionResult::RK_Declaration:
   case CodeCompletionResult::RK_Pattern: {
-llvm::SmallString<128> USR;
-if (/*Ignore=*/clang::index::generateUSRForDecl(R.Declaration, USR))
-  return None;
-return SymbolID(USR);
+return clang::clangd::getSymbolID(R.Declaration);
   }
   case CodeCompletionResult::RK_Macro:
 // FIXME: Macros do have USRs, but the CCR doesn't contain enough info.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added reviewers: ilya-biryukov, ioeric.
Herald added subscribers: arphaman, mgrang, jkorous, MaskRay, mgorny.

This patch implements a SymbolOccurenceCollector, which will be used to:

- Find all occurrences in AST
- Find all occurrences in MemIndex


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385

Files:
  clangd/CMakeLists.txt
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/SymbolOccurrenceCollector.cpp
  clangd/index/SymbolOccurrenceCollector.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/SymbolOccurrenceCollectorTests.cpp

Index: unittests/clangd/SymbolOccurrenceCollectorTests.cpp
===
--- /dev/null
+++ unittests/clangd/SymbolOccurrenceCollectorTests.cpp
@@ -0,0 +1,168 @@
+//===-- SymbolOccurrenceCollectorTests.cpp  -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "index/SymbolOccurrenceCollector.h"
+#include "Annotations.h"
+#include "TestFS.h"
+#include "TestTU.h"
+#include "clang/Index/IndexingAction.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Index/IndexingAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include 
+#include 
+
+MATCHER(OccurrenceRange, "") {
+  const clang::clangd::SymbolOccurrence& Pos = testing::get<0>(arg);
+  const clang::clangd::Range& Range = testing::get<1>(arg);
+  return std::tie(Pos.Location.Start.Line,
+  Pos.Location.Start.Column,
+  Pos.Location.End.Line,
+  Pos.Location.End.Column) ==
+ std::tie(Range.start.line, Range.start.character, Range.end.line,
+  Range.end.character);
+}
+
+namespace clang {
+namespace clangd {
+namespace {
+
+class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
+public:
+  SymbolIndexActionFactory() = default;
+
+  clang::FrontendAction *create() override {
+index::IndexingOptions IndexOpts;
+IndexOpts.SystemSymbolFilter =
+index::IndexingOptions::SystemSymbolFilterKind::All;
+IndexOpts.IndexFunctionLocals = true;
+SymbolOccurrenceKind Filter = SymbolOccurrenceKind::Declaration |
+  SymbolOccurrenceKind::Definition |
+  SymbolOccurrenceKind::Reference;
+SymbolOccurrenceCollector::Options Opts;
+Opts.Filter = Filter;
+Opts.IDs = llvm::None;
+Collector = std::make_shared(Opts);
+return index::createIndexingAction(Collector, IndexOpts, nullptr).release();
+  }
+
+  std::shared_ptr Collector;
+};
+
+class OccurrenceCollectorTest : public ::testing::Test {
+public:
+  OccurrenceCollectorTest()
+  : InMemoryFileSystem(new vfs::InMemoryFileSystem),
+TestHeaderName(testPath("symbol.h")),
+TestFileName(testPath("symbol.cc")) {
+TestHeaderURI = URI::createFile(TestHeaderName).toString();
+TestFileURI = URI::createFile(TestFileName).toString();
+  }
+
+  bool collectOccurrences(StringRef HeaderCode, StringRef MainCode,
+  const std::vector &ExtraArgs = {}) {
+llvm::IntrusiveRefCntPtr Files(
+new FileManager(FileSystemOptions(), InMemoryFileSystem));
+
+auto Factory = llvm::make_unique();
+
+std::vector Args = {
+"symbol_occurrence_collector", "-fsyntax-only", "-xc++",
+"-std=c++11",   "-include",  TestHeaderName};
+Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+// This allows to override the "-xc++" with something else, i.e.
+// -xobjective-c++.
+Args.push_back(TestFileName);
+
+tooling::ToolInvocation Invocation(
+Args,
+Factory->create(), Files.get(),
+std::make_shared());
+
+InMemoryFileSystem->addFile(TestHeaderName, 0,
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+InMemoryFileSystem->addFile(TestFileName, 0,
+llvm::MemoryBuffer::getMemBuffer(MainCode));
+Invocation.run();
+Occurrences = Factory->Collector->takeOccurrences();
+return true;
+  }
+
+protected:
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem;
+  std::string TestHeaderName;
+  std::string TestHeaderURI;
+  std::string TestFileName;
+  std::string TestFileURI;
+  std::unique_ptr Occurrences;
+};
+
+std::vector operator+(const std::vector &L,
+ const std::vector &R) {
+  std::vector Result = L;
+  Result.insert(Result.end(), R.begin(), R.end());
+  retur

[PATCH] D50438: [clangd] Sort GoToDefinition results.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: arphaman, mgrang, jkorous, MaskRay, ioeric.

GoToDefinition returns all declaration results (implicit/explicit) that are
in the same location, and the results are returned in arbitrary order.

Some LSP clients defaultly take the first result as the final result, which
might present a bad result (implicit decl) to users.

this patch ranks the result based on whether the declarations are
referenced explicitly/implicitly. We put explicit declarations first.

This also improve the "hover" (which just take the first result) feature
in some cases.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50438

Files:
  clangd/XRefs.cpp
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -311,6 +311,50 @@
   }
 }
 
+TEST(GoToDefinition, Rank) {
+  auto T = Annotations(R"cpp(
+struct $foo1[[Foo]] {
+  $foo2[[Foo]]();
+  $foo3[[Foo]](Foo&&);
+  $foo4[[Foo]](const char*);
+};
+
+Foo $f[[f]]();
+
+void $g[[g]](Foo foo);
+
+void call() {
+  const char* $str[[str]] = "123";
+  Foo a = $1^str;
+  Foo b = Foo($2^str);
+  Foo c = $3^f();
+  $4^g($5^f());
+  g($6^str);
+}
+  )cpp");
+  auto AST = TestTU::withCode(T.code()).build();
+  EXPECT_THAT(findDefinitions(AST, T.point("1")),
+  ElementsAre(RangeIs(T.range("str")), RangeIs(T.range("foo4";
+  EXPECT_THAT(findDefinitions(AST, T.point("2")),
+  ElementsAre(RangeIs(T.range("str";
+  EXPECT_THAT(findDefinitions(AST, T.point("3")),
+  ElementsAre(RangeIs(T.range("f")), RangeIs(T.range("foo3";
+  EXPECT_THAT(findDefinitions(AST, T.point("4")),
+  ElementsAre(RangeIs(T.range("g";
+  EXPECT_THAT(findDefinitions(AST, T.point("5")),
+  ElementsAre(RangeIs(T.range("f")),
+  RangeIs(T.range("foo3";
+
+  auto DefinitionAtPoint6 = findDefinitions(AST, T.point("6"));
+  EXPECT_EQ(3ul, DefinitionAtPoint6.size());
+  EXPECT_THAT(
+  DefinitionAtPoint6,
+  HasSubsequence(RangeIs(T.range("str")), RangeIs(T.range("foo4";
+  EXPECT_THAT(
+  DefinitionAtPoint6,
+  HasSubsequence(RangeIs(T.range("str")), RangeIs(T.range("foo3";
+}
+
 TEST(GoToDefinition, RelPathsInCompileCommand) {
   Annotations SourceAnnotations(R"cpp(
 int [[foo]];
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -68,10 +68,24 @@
   const MacroInfo *Info;
 };
 
+struct DeclInfo {
+  const Decl *D;
+  // Indicates the declaration is referenced by an explicit AST node.
+  bool IsReferencedExplicitly;
+  bool operator==(const DeclInfo &L) {
+return std::tie(D, IsReferencedExplicitly) ==
+   std::tie(L.D, L.IsReferencedExplicitly);
+  }
+};
+
 /// Finds declarations locations that a given source location refers to.
 class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
-  std::vector Decls;
   std::vector MacroInfos;
+  // The value of the map indicates whether the declaration has been referenced
+  // explicitly in the code.
+  // True means the declaration is explicitly referenced at least once; false
+  // otherwise
+  llvm::DenseMap Decls;
   const SourceLocation &SearchedLocation;
   const ASTContext *
   Preprocessor &PP;
@@ -82,13 +96,17 @@
  ASTContext &AST, Preprocessor &PP)
   : SearchedLocation(SearchedLocation), AST(AST), PP(PP) {}
 
-  std::vector takeDecls() {
-// Don't keep the same declaration multiple times.
-// This can happen when nodes in the AST are visited twice.
-std::sort(Decls.begin(), Decls.end());
-auto Last = std::unique(Decls.begin(), Decls.end());
-Decls.erase(Last, Decls.end());
-return std::move(Decls);
+  std::vector getDecls() const {
+std::vector Result;
+for (auto It : Decls)
+  Result.push_back({It.first, It.second});
+
+// Sort results. Declarations being referenced explicitly come first.
+std::sort(Result.begin(), Result.end(),
+  [](const DeclInfo &L, const DeclInfo &R) {
+return L.IsReferencedExplicitly > R.IsReferencedExplicitly;
+  });
+return Result;
   }
 
   std::vector takeMacroInfos() {
@@ -106,21 +124,45 @@
 return std::move(MacroInfos);
   }
 
+  void insertDecl(const Decl *D, bool IsExplicitReferenced) {
+auto It = Decls.find(D);
+if (It != Decls.end())
+  It->getSecond() |= IsExplicitReferenced;
+else
+  Decls.insert({D, IsExplicitReferenced});
+  }
+
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
   ArrayRef Relations,
   SourceLocation Loc,
   index::IndexDataConsumer::ASTNodeInf

[PATCH] D50389: [clang-tidy] new check for Abseil

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for contributing to clang-tidy!




Comment at: docs/clang-tidy/checks/abseil-duration-division.rst:3
+  
+abseil-duration-division
+

This is a nice document. Does abseil have similar thing in its official 
guideline/practice? If yes, we can add the link in the document as well.



Comment at: test/clang-tidy/abseil-duration-division.cpp:21
+#define CHECK(x) (x)
+
+void Positives() {

JonasToth wrote:
> What happens on this snippet:
> 
> ```
> const auto SomeVal = 1.0 + d / d; // auto deduces to a double?! so it should 
> be considered as relevant
> const auto AnotherVal = 1 + d / d; // Here everything will be an integer, so 
> interesting as well
> ```
> 
+1, we need to add more test cases.


https://reviews.llvm.org/D50389



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

[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added reviewers: ilya-biryukov, alexfh.
Herald added a subscriber: xazax.hun.

The upstream change r336737 causes a regression issue in our internal
base codebase.

Adding an option to the check allowing us whitelist these classes.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50447

Files:
  clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tidy/performance/ForRangeCopyCheck.h
  docs/clang-tidy/checks/performance-for-range-copy.rst
  test/clang-tidy/performance-for-range-copy.cpp


Index: test/clang-tidy/performance-for-range-copy.cpp
===
--- test/clang-tidy/performance-for-range-copy.cpp
+++ test/clang-tidy/performance-for-range-copy.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-for-range-copy %t -- -- -std=c++11 
-fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s performance-for-range-copy %t 
-config="{CheckOptions: [{key: "performance-for-range-copy.WhitelistClasses", 
value: "WhitelistFoo"}]}" -- -std=c++11 -fno-delayed-template-parsing
 
 namespace std {
 
@@ -260,3 +260,11 @@
 bool result = ConstOperatorInvokee != Mutable();
   }
 }
+
+struct WhitelistFoo {
+  ~WhitelistFoo();
+};
+void IgnoreBlacklistedType() {
+  for (auto _ : View>()) {
+  }
+}
Index: docs/clang-tidy/checks/performance-for-range-copy.rst
===
--- docs/clang-tidy/checks/performance-for-range-copy.rst
+++ docs/clang-tidy/checks/performance-for-range-copy.rst
@@ -25,3 +25,8 @@
 
When non-zero, warns on any use of `auto` as the type of the range-based for
loop variable. Default is `0`.
+
+.. option:: WhitelistClasses
+
+   A semicolon-separated list of names of whitelist classes, which will be
+   ignored by the check. Default is empty.
Index: clang-tidy/performance/ForRangeCopyCheck.h
===
--- clang-tidy/performance/ForRangeCopyCheck.h
+++ clang-tidy/performance/ForRangeCopyCheck.h
@@ -40,6 +40,7 @@
ASTContext &Context);
 
   const bool WarnOnAllAutoCopies;
+  std::vector WhitelistClasses;
 };
 
 } // namespace performance
Index: clang-tidy/performance/ForRangeCopyCheck.cpp
===
--- clang-tidy/performance/ForRangeCopyCheck.cpp
+++ clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -11,6 +11,7 @@
 #include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/TypeTraits.h"
+#include "../utils/OptionsUtils.h"
 
 using namespace clang::ast_matchers;
 
@@ -20,18 +21,28 @@
 
 ForRangeCopyCheck::ForRangeCopyCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
-  WarnOnAllAutoCopies(Options.get("WarnOnAllAutoCopies", 0)) {}
+  WarnOnAllAutoCopies(Options.get("WarnOnAllAutoCopies", 0)),
+  WhitelistClasses(utils::options::parseStringList(
+  Options.get("WhitelistClasses", ""))) {}
 
 void ForRangeCopyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnAllAutoCopies", WarnOnAllAutoCopies);
+  Options.store(Opts, "WhitelistClasses",
+utils::options::serializeStringList(WhitelistClasses));
 }
 
 void ForRangeCopyCheck::registerMatchers(MatchFinder *Finder) {
   // Match loop variables that are not references or pointers or are already
   // initialized through MaterializeTemporaryExpr which indicates a type
   // conversion.
+  auto WhitelistClassMatcher =
+  cxxRecordDecl(hasAnyName(SmallVector(
+  WhitelistClasses.begin(), WhitelistClasses.end(;
+  auto WhitelistType = hasUnqualifiedDesugaredType(
+  recordType(hasDeclaration(WhitelistClassMatcher)));
   auto LoopVar = varDecl(
-  hasType(hasCanonicalType(unless(anyOf(referenceType(), pointerType(),
+  hasType(hasCanonicalType(
+  unless(anyOf(referenceType(), pointerType(), WhitelistType,
   unless(hasInitializer(expr(hasDescendant(materializeTemporaryExpr());
   Finder->addMatcher(cxxForRangeStmt(hasLoopVariable(LoopVar.bind("loopVar")))
  .bind("forRange"),


Index: test/clang-tidy/performance-for-range-copy.cpp
===
--- test/clang-tidy/performance-for-range-copy.cpp
+++ test/clang-tidy/performance-for-range-copy.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-for-range-copy %t -- -- -std=c++11 -fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s performance-for-range-copy %t -config="{CheckOptions: [{key: "performance-for-range-copy.WhitelistClasses", value: "WhitelistFoo"}]}" -- -std=c++11 -fno-delayed-template-parsing
 
 namespace std {
 
@@ -260,3 +260,11 @@
 bool result = ConstOperatorInvokee != Mutable();
   }
 }
+
+struct WhitelistFoo {
+  ~WhitelistFoo();
+};
+void IgnoreBlacklistedType() {
+  for 

[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D50447#1192280, @JonasToth wrote:

> If whitelisting works, thats fine. But I agree with @lebedev.ri that a 
> contribution/improvement to the ExprMutAnalyzer is the better option. This is 
> especially the case, because multiple checks (and maybe even some other parts 
> then clang-tidy) will utilize this analysis.


I'm sorry for not explaining it with more details. Might be "regression" is a 
confusing world :(. It is not false positive. The change using ExprMutAnalyzer 
is reasonable, IMO. It makes this check smarter to catch cases which will not 
be caught before. For example,

  for (auto widget : container) {
widget.call_const_method(); // const usage recongized by ExprMutAnalyzer, 
so it is fine to change widget to `const auto&`
  } 

But in our codebase, we have code intended to use like below, and it is in base 
library, and is used widely. I think it makes sense to whitelist this class in 
our internal configuration.

  for (auto _ : state) {
 ... // no `_` being referenced in the for-loop body
  }


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50447



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

[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

> That looks remarkably like google benchmark main loop. (i don't know at hand 
> if making this const will break it, but probably not?)
>  I wonder if instead there should be an option to not complain about the 
> variables that aren't actually used?

Yeah, it is google benchmark library.

The new fix `for (const auto& _ : state)` will trigger -Wunused warning. 
`__attribute__((unused))` doesn't help to suppress the warning :(

  $ cat /tmp/main4.cc 
  #include 
  
  struct __attribute__((unused)) Foo {
  };
  
  void f() {
std::vector foos;
for (const Foo& _ : foos) {
}
  }
  $ g++ /tmp/main4.cc -Wunused  

   
  /tmp/main4.cc: In function ‘void f()’:
  /tmp/main4.cc:8:19: warning: unused variable ‘_’ [-Wunused-variable]
 for (const Foo& _ : foos) {
 ^



> Is the type important, or specifics about the variable (e.g. the name?)
>  The _ variable names are sometimes used for RAII variables/lambdas
>  that shall just do some cleanup, e.g. gsl::finally().
> 
> It might make sense to give ExprMutAnalyzer an ignore-mechanism.
> 
> I wonder if instead there should be an option to not complain about the 
> variables that aren't actually used?
> 
> Checking for variable usage would be simple. The ExprMutAnalyzer
>  always analyses the scope, e.g. a function. You can match this scope for
>  a DeclRefExpr of the variable, empty result means no usage.

Both type and variable name "_" can fix the issue here, but the variable name 
seems a fragile way (if the name is changed, things will be broken again).

Yeah, adding an ignore mechanism to ExprMutAnalyzer is another option.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50447



___
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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

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

A rough round of review.

> 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).




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);

Why rename this function?

 Is it guaranteed that `RealPath` is always an absolute path?



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.

Can we use `RelPathPrefix.empty()` instead of comparing with `StringRef()`?



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

It seems that there is no difference between `HeaderInPreambleAnnotations` and 
`HeaderNotInPreambleAnnotations` in the test. Both of them are treated equally.



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] D50385: [clangd] Collect symbol occurrences from AST.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D50385#1191914, @ioeric wrote:

> 2 high-level questions:
>
> 1. What's the reason for having a separate `SymbolOccurrenceSlab`? Could 
> store occurrences as extra payload of `Symbol`?


Storing occurrences in `Symbol` structure is easy to misuse by users IMO -- if 
we go through this way, we will end up having a `getOccurrences`-like method in 
`Symbol` structure. Once users get the `Symbol` instance, it is natural for 
them to call `getOccurrences` to get all occurrences of the symbol. However 
this `getOccurrences` method doesn't do what users expected (just returning an 
incomplete set of results or empty). To query the symbol occurrences, we should 
always use index interface.

Therefore, I think we should try to avoid these confusions in the design.

> 2. Could we merge `SymbolOccurrenceCollector` into the existing 
> `SymbolCollector`? They look a lot alike. Having another index data consumer 
> seems like more overhead on the user side.

The `SymbolOccurrenceCollector` has many responsibilities (collecting 
declaration, definition, code completion information etc), and the code is 
growing complex now. Merging the `SymbolOccurrenceCollector` to it will make it 
more complicated -- we will introduce more option flags like 
`collect-symbol-only`, `collect-occurrence-only` to configure it for our 
different use cases (we need to the implementation detail clearly in order to 
make a correct option for `SymbolCollector`). And I can foresee these two 
collectors might be run at different point (runWithPreamble vs runWithAST) in 
dynamic index.

They might use same facilities, but we could always share them.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



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

[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 159919.
hokein marked an inline comment as done.
hokein added a comment.

Adress review comment - ignore the case in the check.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50447

Files:
  clang-tidy/performance/ForRangeCopyCheck.cpp
  test/clang-tidy/performance-for-range-copy.cpp


Index: test/clang-tidy/performance-for-range-copy.cpp
===
--- test/clang-tidy/performance-for-range-copy.cpp
+++ test/clang-tidy/performance-for-range-copy.cpp
@@ -260,3 +260,8 @@
 bool result = ConstOperatorInvokee != Mutable();
   }
 }
+
+void IgnoreLoopVariableNotUsedInLoopBody() {
+  for (auto _ : View>()) {
+  }
+}
Index: clang-tidy/performance/ForRangeCopyCheck.cpp
===
--- clang-tidy/performance/ForRangeCopyCheck.cpp
+++ clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -8,6 +8,7 @@
 
//===--===//
 
 #include "ForRangeCopyCheck.h"
+#include "../utils/DeclRefExprUtils.h"
 #include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/TypeTraits.h"
@@ -79,15 +80,27 @@
   utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
 return false;
-  if (utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
-  .isMutated(&LoopVar))
-return false;
-  diag(LoopVar.getLocation(),
-   "loop variable is copied but only used as const reference; consider "
-   "making it a const reference")
-  << utils::fixit::changeVarDeclToConst(LoopVar)
-  << utils::fixit::changeVarDeclToReference(LoopVar, Context);
-  return true;
+  // We omit the case where the loop variable is not used in the loop body. 
E.g.
+  //
+  // for (auto _ : benchmark_state) {
+  // }
+  //
+  // Because the fix (changing to `const auto &`) will introduce an unused
+  // compiler warning which can't be suppressed.
+  // Since this case is very rare, it is safe to ignore it.
+  if (!utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
+  .isMutated(&LoopVar) &&
+  !utils::decl_ref_expr::allDeclRefExprs(LoopVar, *ForRange.getBody(),
+ Context)
+   .empty()) {
+diag(LoopVar.getLocation(),
+ "loop variable is copied but only used as const reference; consider "
+ "making it a const reference")
+<< utils::fixit::changeVarDeclToConst(LoopVar)
+<< utils::fixit::changeVarDeclToReference(LoopVar, Context);
+return true;
+  }
+  return false;
 }
 
 } // namespace performance


Index: test/clang-tidy/performance-for-range-copy.cpp
===
--- test/clang-tidy/performance-for-range-copy.cpp
+++ test/clang-tidy/performance-for-range-copy.cpp
@@ -260,3 +260,8 @@
 bool result = ConstOperatorInvokee != Mutable();
   }
 }
+
+void IgnoreLoopVariableNotUsedInLoopBody() {
+  for (auto _ : View>()) {
+  }
+}
Index: clang-tidy/performance/ForRangeCopyCheck.cpp
===
--- clang-tidy/performance/ForRangeCopyCheck.cpp
+++ clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -8,6 +8,7 @@
 //===--===//
 
 #include "ForRangeCopyCheck.h"
+#include "../utils/DeclRefExprUtils.h"
 #include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/TypeTraits.h"
@@ -79,15 +80,27 @@
   utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
 return false;
-  if (utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
-  .isMutated(&LoopVar))
-return false;
-  diag(LoopVar.getLocation(),
-   "loop variable is copied but only used as const reference; consider "
-   "making it a const reference")
-  << utils::fixit::changeVarDeclToConst(LoopVar)
-  << utils::fixit::changeVarDeclToReference(LoopVar, Context);
-  return true;
+  // We omit the case where the loop variable is not used in the loop body. E.g.
+  //
+  // for (auto _ : benchmark_state) {
+  // }
+  //
+  // Because the fix (changing to `const auto &`) will introduce an unused
+  // compiler warning which can't be suppressed.
+  // Since this case is very rare, it is safe to ignore it.
+  if (!utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
+  .isMutated(&LoopVar) &&
+  !utils::decl_ref_expr::allDeclRefExprs(LoopVar, *ForRange.getBody(),
+ Context)
+   .empty()) {
+diag(LoopVar.getLocation(),
+ "loop variable is copied but only used as const reference; consider "
+ "making it a const reference")
+

[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:39
+  auto WhitelistClassMatcher =
+  cxxRecordDecl(hasAnyName(SmallVector(
+  WhitelistClasses.begin(), WhitelistClasses.end(;

JonasToth wrote:
> I have seens this pattern now multiple times in various checks, we should 
> have some utility wrapping the `hasAnyName(MyList)`. (Just in general, does 
> not need to happen with this check)
no needed for this patch. But yes! Moving to utility is reasonable to me.



Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:93
 return false;
   if (utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
   .isMutated(&LoopVar))

JonasToth wrote:
> Adding a `..IsMutated(&LoopVar) && IsReferenced(ForScope, LoopVar))` here 
> should fix the warning as well.
I think ignoring `for (auto _ : state)` is a sensible solution. Thanks!



Comment at: test/clang-tidy/performance-for-range-copy.cpp:1
-// RUN: %check_clang_tidy %s performance-for-range-copy %t -- -- -std=c++11 
-fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s performance-for-range-copy %t 
-config="{CheckOptions: [{key: "performance-for-range-copy.WhitelistClasses", 
value: "WhitelistFoo"}]}" -- -std=c++11 -fno-delayed-template-parsing
 

JonasToth wrote:
> I would prefer a single file, that has the configuration and leave the 
> standard test like it is.
> 
> with this, you can always track what is actually done by default and what is 
> done with different conigurations + potential inconsistencies that might 
> occur by bugs in the configured code.
no needed this configuration now.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50447



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

[PATCH] D50447: [clang-tidy] Omit cases where loop variable is not used in loop body in performance-for-range-copy.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339415: [clang-tidy] Omit cases where loop variable is not 
used in loop body in (authored by hokein, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D50447

Files:
  clang-tools-extra/trunk/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/trunk/test/clang-tidy/performance-for-range-copy.cpp


Index: clang-tools-extra/trunk/clang-tidy/performance/ForRangeCopyCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -8,6 +8,7 @@
 
//===--===//
 
 #include "ForRangeCopyCheck.h"
+#include "../utils/DeclRefExprUtils.h"
 #include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/TypeTraits.h"
@@ -79,15 +80,27 @@
   utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
 return false;
-  if (utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
-  .isMutated(&LoopVar))
-return false;
-  diag(LoopVar.getLocation(),
-   "loop variable is copied but only used as const reference; consider "
-   "making it a const reference")
-  << utils::fixit::changeVarDeclToConst(LoopVar)
-  << utils::fixit::changeVarDeclToReference(LoopVar, Context);
-  return true;
+  // We omit the case where the loop variable is not used in the loop body. 
E.g.
+  //
+  // for (auto _ : benchmark_state) {
+  // }
+  //
+  // Because the fix (changing to `const auto &`) will introduce an unused
+  // compiler warning which can't be suppressed.
+  // Since this case is very rare, it is safe to ignore it.
+  if (!utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
+  .isMutated(&LoopVar) &&
+  !utils::decl_ref_expr::allDeclRefExprs(LoopVar, *ForRange.getBody(),
+ Context)
+   .empty()) {
+diag(LoopVar.getLocation(),
+ "loop variable is copied but only used as const reference; consider "
+ "making it a const reference")
+<< utils::fixit::changeVarDeclToConst(LoopVar)
+<< utils::fixit::changeVarDeclToReference(LoopVar, Context);
+return true;
+  }
+  return false;
 }
 
 } // namespace performance
Index: clang-tools-extra/trunk/test/clang-tidy/performance-for-range-copy.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/performance-for-range-copy.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/performance-for-range-copy.cpp
@@ -260,3 +260,8 @@
 bool result = ConstOperatorInvokee != Mutable();
   }
 }
+
+void IgnoreLoopVariableNotUsedInLoopBody() {
+  for (auto _ : View>()) {
+  }
+}


Index: clang-tools-extra/trunk/clang-tidy/performance/ForRangeCopyCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -8,6 +8,7 @@
 //===--===//
 
 #include "ForRangeCopyCheck.h"
+#include "../utils/DeclRefExprUtils.h"
 #include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/TypeTraits.h"
@@ -79,15 +80,27 @@
   utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
 return false;
-  if (utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
-  .isMutated(&LoopVar))
-return false;
-  diag(LoopVar.getLocation(),
-   "loop variable is copied but only used as const reference; consider "
-   "making it a const reference")
-  << utils::fixit::changeVarDeclToConst(LoopVar)
-  << utils::fixit::changeVarDeclToReference(LoopVar, Context);
-  return true;
+  // We omit the case where the loop variable is not used in the loop body. E.g.
+  //
+  // for (auto _ : benchmark_state) {
+  // }
+  //
+  // Because the fix (changing to `const auto &`) will introduce an unused
+  // compiler warning which can't be suppressed.
+  // Since this case is very rare, it is safe to ignore it.
+  if (!utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
+  .isMutated(&LoopVar) &&
+  !utils::decl_ref_expr::allDeclRefExprs(LoopVar, *ForRange.getBody(),
+ Context)
+   .empty()) {
+diag(LoopVar.getLocation(),
+ "loop variable is copied but only used as const reference; consider "
+ "making it a const reference")
+<< utils::fixit::changeVarDeclToConst(LoopVar)
+   

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

[Haojian Wu via Phabricator via cfe-commits]

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

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!




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);

simark wrote:
> 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.
That makes sense, thanks for the explanation and the useful link!



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

nit: this comment is not needed.



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

simark wrote:
> 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.
> ```
SG.


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] D50389: [clang-tidy] Abseil: integral division of Duration check

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:24
+
+  const auto IsDuration =
+  expr(hasType(cxxRecordDecl(hasName("::absl::Duration";

maybe call it `DurationExpr` since you have declared the variable as 
`expr(...)`.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:30
+  hasOverloadedOperatorName("/"),
+  hasArgument(0, expr(IsDuration)),
+  hasArgument(1, expr(IsDuration))).bind("OpCall"))),

`expr(IsDuration)` seems have a duplicated `expr`, isn't `hasArgument(0, 
IsDuration())` enough?



Comment at: test/clang-tidy/abseil-duration-division.cpp:18
+
+absl::Duration d;
+

could you make it to a local variable? It willmake the test code easier to 
read, otherwise readers have to scroll up the file to see what is variable `d`.



Comment at: test/clang-tidy/abseil-duration-division.cpp:58
+  // CHECK-MESSAGES: [[@LINE-4]]:45: warning: operator/ on absl::Duration 
objects
+  // CHECK-FIXES: double DoubleDivision(T t1, T t2) {return
+  // absl::FDivDuration(t1, t2);}

I think we should ignore templates. The template function could be used by 
other types, fixing the template is not correct. 


https://reviews.llvm.org/D50389



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

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

The check is missing its document, please add one in `docs/clang-tidy/checks/`.




Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+  Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);

aaron.ballman wrote:
> I think this needs a `not(isExpansionInSystemHeader())` in there, as this is 
> going to trigger on code that includes a header file using an `absl` 
> namespace. If I'm incorrect and users typically include abseil as something 
> other than system includes, you'll have to find a different way to solve this.
Yeah, we have discussed this issue internally.  abseil-user code usually 
includes abseil header like `#include "whatever/abseil/base/optimization.h"`, 
so `IsExpansionInSystemHeader` doesn't work for most cases. 

We need a way to filter out all headers being a part of abseil library. I think 
we can create a matcher `InExpansionInAbseilHeader` -- basically using 
`IsExpansionInFileMatching` with a regex expression that matches typical abseil 
include path. What do you think?

And we'll have more abseil checks (e.g.  `abseil-no-internal-deps`) that use 
`InExpansionInAbseilHeader`, we should put it to a common header.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50580



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

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:6
+
+Gives a warning if code using Abseil depends on internal details. If something 
is in a namespace or filename/path that includes the word “internal”, code is 
not allowed to depend upon it beaucse it’s an implementation detail. They 
cannot friend it, include it, you mention it or refer to it in any way. Doing 
so violtaes Abseil's compatibility guidelines and may result in breakage.
+

aaron.ballman wrote:
> JonasToth wrote:
> > s/violtaes/violates/
> Please wrap lines to 80 cols.
> 
> Nothing in this check looks at filenames and paths, but this suggests the 
> check will find those. Is that intended work that's missed in this patch, or 
> are the docs incorrect?
Please provide a link for the abseil guideline.



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:2
+// RUN: %check_clang_tidy %s abseil-no-internal-deps %t
+
+

nit: please make sure the code follow LLVM code style, even for test code :)



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:11
+
+namespace absl {
+std::string StringsFunction (std::string s1){

Since we have multiple abseil checks that might use these fake abseil 
declarations, I'd suggest pull out these to a common header, and include it in 
this test file.


https://reviews.llvm.org/D50542



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

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: arphaman, jkorous, MaskRay, ioeric, javed.absar.

clangd maintains the last good preamble for each TU and clang treats an empty
preamble as an error,  therefore, clangd will use the stale preamble for
a latest AST, which causes some wired issues for the features using AST
(e.g. GoToDefintion).

A testcase:

1. Open a file contains some includes (like "#include ") in clangd
2. Change the whole content of the file with the source without includes (like 
"double funcc()")
3. Call findDefinition in "double fun^cc()", it will goes to #include file 
vector -- because we discard the empty preamble built in step2, and still hold 
the stale preamble in step1


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50627

Files:
  unittests/clangd/TUSchedulerTests.cpp


Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -308,6 +308,49 @@
   UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
+TEST_F(TUSchedulerTests, EmptyPreamble) {
+  TUScheduler S(
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+  PreambleParsedCallback(),
+  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+  ASTRetentionPolicy());
+
+  auto Foo = testPath("foo.cpp");
+  auto Header = testPath("foo.h");
+
+  Files[Header] = "void foo()";
+  Timestamps[Header] = time_t(0);
+  auto WithPreamble = R"cpp(
+#include "foo.h"
+int main() {}
+  )cpp";
+  auto WithEmptyPreamble = R"cpp(int main() {})cpp";
+  S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  S.runWithPreamble("getNonEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get a non-empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_GT(Preamble->Preamble->Preamble.getBounds().Size,
+0u);
+});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+  // Update the file which results in an empty preamble.
+  S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+  S.runWithPreamble("getEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get an empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_EQ(Preamble->Preamble->Preamble.getBounds().Size,
+0u);
+});
+}
+
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.


Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -308,6 +308,49 @@
   UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
+TEST_F(TUSchedulerTests, EmptyPreamble) {
+  TUScheduler S(
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+  PreambleParsedCallback(),
+  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+  ASTRetentionPolicy());
+
+  auto Foo = testPath("foo.cpp");
+  auto Header = testPath("foo.h");
+
+  Files[Header] = "void foo()";
+  Timestamps[Header] = time_t(0);
+  auto WithPreamble = R"cpp(
+#include "foo.h"
+int main() {}
+  )cpp";
+  auto WithEmptyPreamble = R"cpp(int main() {})cpp";
+  S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  S.runWithPreamble("getNonEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get a non-empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_GT(Preamble->Preamble->Preamble.getBounds().Size,
+0u);
+});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+  // Update the file which results in an empty preamble.
+  S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+  S.runWithPreamble("getEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get an empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_EQ(Preamble->Preamble->Pream

[PATCH] D50628: [Preamble] Empty preamble is not an error.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.

Empty preamble is valid for source file which doesn't have any
preprocessor and #includes.

This patch makes clang treat an empty preamble as a normal preamble.

Check: ninja check-clang

A testcase is added in https://reviews.llvm.org/D50627.


Repository:
  rC Clang

https://reviews.llvm.org/D50628

Files:
  include/clang/Frontend/PrecompiledPreamble.h
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/PrecompiledPreamble.cpp


Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -237,9 +237,6 @@
 PreambleCallbacks &Callbacks) {
   assert(VFS && "VFS is null");
 
-  if (!Bounds.Size)
-return BuildPreambleError::PreambleIsEmpty;
-
   auto PreambleInvocation = std::make_shared(Invocation);
   FrontendOptions &FrontendOpts = PreambleInvocation->getFrontendOpts();
   PreprocessorOptions &PreprocessorOpts =
@@ -423,9 +420,6 @@
   PreprocessorOptions &PreprocessorOpts =
   PreambleInvocation->getPreprocessorOpts();
 
-  if (!Bounds.Size)
-return false;
-
   // We've previously computed a preamble. Check whether we have the same
   // preamble now that we did before, and that there's enough space in
   // the main-file buffer within the precompiled preamble to fit the
@@ -758,8 +752,6 @@
 
 std::string BuildPreambleErrorCategory::message(int condition) const {
   switch (static_cast(condition)) {
-  case BuildPreambleError::PreambleIsEmpty:
-return "Preamble is empty";
   case BuildPreambleError::CouldntCreateTempFile:
 return "Could not create temporary file for PCH";
   case BuildPreambleError::CouldntCreateTargetInfo:
Index: lib/Frontend/ASTUnit.cpp
===
--- lib/Frontend/ASTUnit.cpp
+++ lib/Frontend/ASTUnit.cpp
@@ -1363,7 +1363,6 @@
 } else {
   switch (static_cast(NewPreamble.getError().value())) 
{
   case BuildPreambleError::CouldntCreateTempFile:
-  case BuildPreambleError::PreambleIsEmpty:
 // Try again next time.
 PreambleRebuildCounter = 1;
 return nullptr;
Index: include/clang/Frontend/PrecompiledPreamble.h
===
--- include/clang/Frontend/PrecompiledPreamble.h
+++ include/clang/Frontend/PrecompiledPreamble.h
@@ -286,8 +286,7 @@
 };
 
 enum class BuildPreambleError {
-  PreambleIsEmpty = 1,
-  CouldntCreateTempFile,
+  CouldntCreateTempFile = 1,
   CouldntCreateTargetInfo,
   BeginSourceFileFailed,
   CouldntEmitPCH


Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -237,9 +237,6 @@
 PreambleCallbacks &Callbacks) {
   assert(VFS && "VFS is null");
 
-  if (!Bounds.Size)
-return BuildPreambleError::PreambleIsEmpty;
-
   auto PreambleInvocation = std::make_shared(Invocation);
   FrontendOptions &FrontendOpts = PreambleInvocation->getFrontendOpts();
   PreprocessorOptions &PreprocessorOpts =
@@ -423,9 +420,6 @@
   PreprocessorOptions &PreprocessorOpts =
   PreambleInvocation->getPreprocessorOpts();
 
-  if (!Bounds.Size)
-return false;
-
   // We've previously computed a preamble. Check whether we have the same
   // preamble now that we did before, and that there's enough space in
   // the main-file buffer within the precompiled preamble to fit the
@@ -758,8 +752,6 @@
 
 std::string BuildPreambleErrorCategory::message(int condition) const {
   switch (static_cast(condition)) {
-  case BuildPreambleError::PreambleIsEmpty:
-return "Preamble is empty";
   case BuildPreambleError::CouldntCreateTempFile:
 return "Could not create temporary file for PCH";
   case BuildPreambleError::CouldntCreateTargetInfo:
Index: lib/Frontend/ASTUnit.cpp
===
--- lib/Frontend/ASTUnit.cpp
+++ lib/Frontend/ASTUnit.cpp
@@ -1363,7 +1363,6 @@
 } else {
   switch (static_cast(NewPreamble.getError().value())) {
   case BuildPreambleError::CouldntCreateTempFile:
-  case BuildPreambleError::PreambleIsEmpty:
 // Try again next time.
 PreambleRebuildCounter = 1;
 return nullptr;
Index: include/clang/Frontend/PrecompiledPreamble.h
===
--- include/clang/Frontend/PrecompiledPreamble.h
+++ include/clang/Frontend/PrecompiledPreamble.h
@@ -286,8 +286,7 @@
 };
 
 enum class BuildPreambleError {
-  PreambleIsEmpty = 1,
-  CouldntCreateTempFile,
+  CouldntCreateTempFile = 1,
   CouldntCreateTargetInfo,
   BeginSourceFileFailed,
   CouldntEmitPCH
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Hello, this patch seems introduce a new crash, and I have reverted it in 
r339558.

Here is the minimal test case:

  class SCOPED_LOCKABLE FileLock {
   public:
explicit FileLock()
EXCLUSIVE_LOCK_FUNCTION(file_);
~FileLock() UNLOCK_FUNCTION(file_);
//void Release() UNLOCK_FUNCTION(file_);
void Lock() EXCLUSIVE_LOCK_FUNCTION(file_);
Mutex file_;
  };
  
  void relockShared2() {
FileLock file_lock;
file_lock.Lock();
  }


Repository:
  rC Clang

https://reviews.llvm.org/D49885



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

[PATCH] D50635: Fix lint tests for D50449

[Haojian Wu via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50635



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

[PATCH] D50645: [clangd] Show non-instantiated decls in signatureHelp

[Haojian Wu via Phabricator via cfe-commits]

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

looks good.




Comment at: clangd/CodeComplete.cpp:721
+  // would get 'std::basic_string').
+  if (auto Func = Candidate.getFunction()) {
+if (auto Pattern = Func->getTemplateInstantiationPattern())

nit: auto*, the same below.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50645



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

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D50627#1196988, @ilya-biryukov wrote:

> Maybe also add a test for find-definition that was broken before? (non-empty 
> preamble -> empty preamble -> bad gotodef that goes to included file instead 
> of the local variable)
>  To have a regression test against similar failures.


I think this patch is in a good scope (for empty preamble). I'd leave it as it 
is. The failure of GoTodefinition test is caused by an inconsistent behavior of 
using `lastBuiltPreamble`/`NewPreamble` in TUScheduler, I will send out a 
separate patch fixing it (based on our discussion).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50627



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

[PATCH] D50695: [clangd] Always use the latest preamble

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: arphaman, jkorous, MaskRay, ioeric, javed.absar.

Fix an inconsistent behavior of using `LastBuiltPreamble`/`NewPreamble`
in TUScheduler (see the test for details), AST should always use
NewPreamble. This patch makes LastBuiltPreamble always point to
NewPreamble.

Preamble rarely fails to build, even there are errors in headers, so we
assume it would not cause performace issue for code completion.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50695

Files:
  clangd/TUScheduler.cpp
  unittests/clangd/XRefsTests.cpp


Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1021,6 +1021,65 @@
   EXPECT_THAT(*Locations, IsEmpty());
 }
 
+TEST(GoToDefinitoin, WithPreamble) {
+  // Test stragety: AST should always use the latest preamble instead of last
+  // good preamble.
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  auto FooCppUri = URIForFile{FooCpp};
+  // The trigger locations must be the same.
+  Annotations FooWithHeader(R"cpp(#include "fo^o.h")cpp");
+  Annotations FooWithoutHeader(R"cpp(double[[fo^o]]();)cpp");
+
+  FS.Files[FooCpp] = FooWithHeader.code();
+
+  auto FooH = testPath("foo.h");
+  auto FooHUri = URIForFile{FooH};
+  Annotations FooHeader(R"cpp([[]])cpp");
+  FS.Files[FooH] = FooHeader.code();
+
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // GoToDefinition goes to a #include file: the result comes from the 
preamble.
+  Server.findDefinitions(
+  FooCpp, FooWithHeader.point(),
+  [&](llvm::Expected> Loc) {
+ASSERT_TRUE(bool(Loc));
+EXPECT_THAT(*Loc, ElementsAre(Location{FooHUri, FooHeader.range()}));
+  });
+
+  // Only preamble is built, and no AST is built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  // We build AST here, and it should use the latest preamble rather than the
+  // stale one.
+  Server.findDefinitions(
+  FooCpp, FooWithoutHeader.point(),
+  [&](llvm::Expected> Loc) {
+ASSERT_TRUE(bool(Loc));
+EXPECT_THAT(*Loc,
+ElementsAre(Location{FooCppUri, 
FooWithoutHeader.range()}));
+  });
+  EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Wait for GoToDefinition";
+
+
+  // Reset test environment.
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // Both preamble and AST are built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  // Use the AST being built in above request.
+  Server.findDefinitions(
+  FooCpp, FooWithoutHeader.point(),
+  [&](llvm::Expected> Loc) {
+ASSERT_TRUE(bool(Loc));
+EXPECT_THAT(*Loc,
+ElementsAre(Location{FooCppUri, 
FooWithoutHeader.range()}));
+  });
+  EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Wait for GoToDefinition";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -370,8 +370,8 @@
 bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
 {
   std::lock_guard Lock(Mutex);
-  if (NewPreamble)
-LastBuiltPreamble = NewPreamble;
+  // Always use the NewPreamble.
+  LastBuiltPreamble = NewPreamble;
 }
 // Before doing the expensive AST reparse, we want to release our reference
 // to the old preamble, so it can be freed if there are no other references


Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1021,6 +1021,65 @@
   EXPECT_THAT(*Locations, IsEmpty());
 }
 
+TEST(GoToDefinitoin, WithPreamble) {
+  // Test stragety: AST should always use the latest preamble instead of last
+  // good preamble.
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  auto FooCppUri = URIForFile{FooCpp};
+  // The trigger locations must be the same.
+  Annotations FooWithHeader(R"cpp(#include "fo^o.h")cpp");
+  Annotations FooWithoutHeader(R"cpp(double[[fo^o]]();)cpp");
+
+  FS.Files[FooCpp] = FooWithHeader.code();
+
+  auto FooH = testPath("foo.h");
+  auto FooHUri = URIForFile{FooH};
+  Annotations FooHeader(R"cpp([[]])cpp");
+  FS.Files[FooH] = FooHeader.code();
+
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // GoToDefinition goes to a #include file: the result comes from the preamble.
+  S

[PATCH] D50438: [clangd] Sort GoToDefinition results.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 160544.
hokein marked 3 inline comments as done.
hokein added a comment.

Address review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50438

Files:
  clangd/XRefs.cpp
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -311,6 +311,50 @@
   }
 }
 
+TEST(GoToDefinition, Rank) {
+  auto T = Annotations(R"cpp(
+struct $foo1[[Foo]] {
+  $foo2[[Foo]]();
+  $foo3[[Foo]](Foo&&);
+  $foo4[[Foo]](const char*);
+};
+
+Foo $f[[f]]();
+
+void $g[[g]](Foo foo);
+
+void call() {
+  const char* $str[[str]] = "123";
+  Foo a = $1^str;
+  Foo b = Foo($2^str);
+  Foo c = $3^f();
+  $4^g($5^f());
+  g($6^str);
+}
+  )cpp");
+  auto AST = TestTU::withCode(T.code()).build();
+  EXPECT_THAT(findDefinitions(AST, T.point("1")),
+  ElementsAre(RangeIs(T.range("str")), RangeIs(T.range("foo4";
+  EXPECT_THAT(findDefinitions(AST, T.point("2")),
+  ElementsAre(RangeIs(T.range("str";
+  EXPECT_THAT(findDefinitions(AST, T.point("3")),
+  ElementsAre(RangeIs(T.range("f")), RangeIs(T.range("foo3";
+  EXPECT_THAT(findDefinitions(AST, T.point("4")),
+  ElementsAre(RangeIs(T.range("g";
+  EXPECT_THAT(findDefinitions(AST, T.point("5")),
+  ElementsAre(RangeIs(T.range("f")),
+  RangeIs(T.range("foo3";
+
+  auto DefinitionAtPoint6 = findDefinitions(AST, T.point("6"));
+  EXPECT_EQ(3ul, DefinitionAtPoint6.size());
+  EXPECT_THAT(
+  DefinitionAtPoint6,
+  HasSubsequence(RangeIs(T.range("str")), RangeIs(T.range("foo4";
+  EXPECT_THAT(
+  DefinitionAtPoint6,
+  HasSubsequence(RangeIs(T.range("str")), RangeIs(T.range("foo3";
+}
+
 TEST(GoToDefinition, RelPathsInCompileCommand) {
   // The source is in "/clangd-test/src".
   // We build in "/clangd-test/build".
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -68,10 +68,24 @@
   const MacroInfo *Info;
 };
 
+struct DeclInfo {
+  const Decl *D;
+  // Indicates the declaration is referenced by an explicit AST node.
+  bool IsReferencedExplicitly;
+  bool operator==(const DeclInfo &L) {
+return std::tie(D, IsReferencedExplicitly) ==
+   std::tie(L.D, L.IsReferencedExplicitly);
+  }
+};
+
 /// Finds declarations locations that a given source location refers to.
 class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
-  std::vector Decls;
   std::vector MacroInfos;
+  // The value of the map indicates whether the declaration has been referenced
+  // explicitly in the code.
+  // True means the declaration is explicitly referenced at least once; false
+  // otherwise
+  llvm::DenseMap Decls;
   const SourceLocation &SearchedLocation;
   const ASTContext *
   Preprocessor &PP;
@@ -82,13 +96,19 @@
  ASTContext &AST, Preprocessor &PP)
   : SearchedLocation(SearchedLocation), AST(AST), PP(PP) {}
 
-  std::vector takeDecls() {
-// Don't keep the same declaration multiple times.
-// This can happen when nodes in the AST are visited twice.
-std::sort(Decls.begin(), Decls.end());
-auto Last = std::unique(Decls.begin(), Decls.end());
-Decls.erase(Last, Decls.end());
-return std::move(Decls);
+  std::vector getDecls() const {
+std::vector Result;
+for (auto It : Decls)
+  Result.push_back({It.first, It.second});
+
+// Sort results. Declarations being referenced explicitly come first.
+std::sort(Result.begin(), Result.end(),
+  [](const DeclInfo &L, const DeclInfo &R) {
+if (L.IsReferencedExplicitly != R.IsReferencedExplicitly)
+  return L.IsReferencedExplicitly > R.IsReferencedExplicitly;
+return L.D < R.D;
+  });
+return Result;
   }
 
   std::vector takeMacroInfos() {
@@ -112,15 +132,30 @@
   SourceLocation Loc,
   index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
 if (Loc == SearchedLocation) {
+  // Check whether the E has an implicit AST node (e.g. ImplicitCastExpr).
+  auto hasImplicitExpr = [](const Expr *E) {
+if (!E || E->child_begin() == E->child_end())
+  return false;
+// Use the first child is good enough for most cases -- normally the
+// expression returned by handleDeclOccurence contains exactly one
+// child expression.
+const auto *FirstChild = *E->child_begin();
+return llvm::isa(FirstChild) ||
+   llvm::isa(FirstChild) ||
+   llvm::isa(FirstChild) ||
+   llvm::isa(FirstChild);
+  };
+
+  bool IsExplicit = !hasImplicitExpr(ASTNode.OrigE);
   // Find and add 

[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Looks mostly good.




Comment at: test/clang-tidy/abseil-duration-division.cpp:58
+  // CHECK-MESSAGES: [[@LINE-4]]:45: warning: operator/ on absl::Duration 
objects
+  // CHECK-FIXES: double DoubleDivision(T t1, T t2) {return
+  // absl::FDivDuration(t1, t2);}

deannagarcia wrote:
> hokein wrote:
> > I think we should ignore templates. The template function could be used by 
> > other types, fixing the template is not correct. 
> I removed this template function, but left the template function TakesGeneric 
> below to show that the automatic type does not require/give a warning. Is 
> that alright?
The check will still give warnings in the template instantiation,  I think we 
need `unless(isInTemplateInstantiation()` matcher to filter them out.


https://reviews.llvm.org/D50389



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

[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check

[Haojian Wu via Phabricator via cfe-commits]

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

Thanks, looks good from my side. I will commit for you if other reviewers have 
no further comments.


https://reviews.llvm.org/D50389



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.

looks good, a few nits.




Comment at: clangd/CodeComplete.cpp:742
+llvm::DenseMap FetchedDocs;
+if (Index) {
+  LookupRequest IndexRequest;

nit: do we want to log anything here? It may be useful for debug.



Comment at: clangd/index/Index.h:65
 public:
+  static llvm::Optional forDecl(const Decl &D);
+

We already have this similar function in clangd/AST.h.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50695: [clangd] Always use the latest preamble

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 160983.
hokein marked 4 inline comments as done.
hokein added a comment.

Address review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50695

Files:
  clangd/TUScheduler.cpp
  unittests/clangd/XRefsTests.cpp


Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1021,6 +1021,51 @@
   EXPECT_THAT(*Locations, IsEmpty());
 }
 
+TEST(GoToDefinition, WithPreamble) {
+  // Test stragety: AST should always use the latest preamble instead of last
+  // good preamble.
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  auto FooCppUri = URIForFile{FooCpp};
+  // The trigger locations must be the same.
+  Annotations FooWithHeader(R"cpp(#include "fo^o.h")cpp");
+  Annotations FooWithoutHeader(R"cpp(double[[fo^o]]();)cpp");
+
+  FS.Files[FooCpp] = FooWithHeader.code();
+
+  auto FooH = testPath("foo.h");
+  auto FooHUri = URIForFile{FooH};
+  Annotations FooHeader(R"cpp([[]])cpp");
+  FS.Files[FooH] = FooHeader.code();
+
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // GoToDefinition goes to a #include file: the result comes from the 
preamble.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithHeader.point())),
+  ElementsAre(Location{FooHUri, FooHeader.range()}));
+
+  // Only preamble is built, and no AST is built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  // We build AST here, and it should use the latest preamble rather than the
+  // stale one.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+  ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+
+  // Reset test environment.
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // Both preamble and AST are built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  // Use the AST being built in above request.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+  ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -370,8 +370,7 @@
 bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
 {
   std::lock_guard Lock(Mutex);
-  if (NewPreamble)
-LastBuiltPreamble = NewPreamble;
+  LastBuiltPreamble = NewPreamble;
 }
 // Before doing the expensive AST reparse, we want to release our reference
 // to the old preamble, so it can be freed if there are no other references


Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1021,6 +1021,51 @@
   EXPECT_THAT(*Locations, IsEmpty());
 }
 
+TEST(GoToDefinition, WithPreamble) {
+  // Test stragety: AST should always use the latest preamble instead of last
+  // good preamble.
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  auto FooCppUri = URIForFile{FooCpp};
+  // The trigger locations must be the same.
+  Annotations FooWithHeader(R"cpp(#include "fo^o.h")cpp");
+  Annotations FooWithoutHeader(R"cpp(double[[fo^o]]();)cpp");
+
+  FS.Files[FooCpp] = FooWithHeader.code();
+
+  auto FooH = testPath("foo.h");
+  auto FooHUri = URIForFile{FooH};
+  Annotations FooHeader(R"cpp([[]])cpp");
+  FS.Files[FooH] = FooHeader.code();
+
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // GoToDefinition goes to a #include file: the result comes from the preamble.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithHeader.point())),
+  ElementsAre(Location{FooHUri, FooHeader.range()}));
+
+  // Only preamble is built, and no AST is built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  // We build AST here, and it should use the latest preamble rather than the
+  // stale one.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+  ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+
+  // Reset test environment.
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // Both preamble and AST are built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  // Use the AST being built in above

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/index/Index.h:65
 public:
+  static llvm::Optional forDecl(const Decl &D);
+

ilya-biryukov wrote:
> hokein wrote:
> > We already have this similar function in clangd/AST.h.
> Thanks for pointing this out.
> It's somewhat hard to find. WDYT about moving it to `Index.h`? The concern 
> would probably be a somewhat broken layering, right? E.g. `Index.h` should 
> not directly depend on anything AST-specific
Yes, I think we will not add any AST-specific stuff to `Index.h`, that's why we 
have `AST.h`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 161000.
hokein marked 2 inline comments as done.
hokein added a comment.

Address review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50627

Files:
  unittests/clangd/TUSchedulerTests.cpp


Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -308,6 +308,52 @@
   UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
+TEST_F(TUSchedulerTests, EmptyPreamble) {
+  TUScheduler S(
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+  PreambleParsedCallback(),
+  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+  ASTRetentionPolicy());
+
+  auto Foo = testPath("foo.cpp");
+  auto Header = testPath("foo.h");
+
+  Files[Header] = "void foo()";
+  Timestamps[Header] = time_t(0);
+  auto WithPreamble = R"cpp(
+#include "foo.h"
+int main() {}
+  )cpp";
+  auto WithEmptyPreamble = R"cpp(int main() {})cpp";
+  S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  S.runWithPreamble("getNonEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get a non-empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_GT(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+  // Update the file which results in an empty preamble.
+  S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+  S.runWithPreamble("getEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get an empty preamble.
+  EXPECT_EQ(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+}
+
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.


Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -308,6 +308,52 @@
   UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
+TEST_F(TUSchedulerTests, EmptyPreamble) {
+  TUScheduler S(
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+  PreambleParsedCallback(),
+  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+  ASTRetentionPolicy());
+
+  auto Foo = testPath("foo.cpp");
+  auto Header = testPath("foo.h");
+
+  Files[Header] = "void foo()";
+  Timestamps[Header] = time_t(0);
+  auto WithPreamble = R"cpp(
+#include "foo.h"
+int main() {}
+  )cpp";
+  auto WithEmptyPreamble = R"cpp(int main() {})cpp";
+  S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  S.runWithPreamble("getNonEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get a non-empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_GT(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+  // Update the file which results in an empty preamble.
+  S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+  S.runWithPreamble("getEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get an empty preamble.
+  EXPECT_EQ(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+}
+
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
___
c

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > drive by: I think this should  be `vlog` or `dlog`.
> > Code completion also logs the number of results from sema, index, etc. 
> > using the `log()` call.
> > The added log message looks similar, so trying to be consistent with the 
> > rest of the code in this file.
> > 
> > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather 
> > leave it to a separate patch.
> I'm not sure which level code completion log messages should be in, but I 
> think we should follow the guidelines in the logger documentation instead of 
> the existing uses.
> 
> Quote from Logger.h
> ```
> // log() is used for information important to understanding a clangd session.
> // e.g. the names of LSP messages sent are logged at this level.
> // This level could be enabled in production builds to allow later inspection.
> 
> // vlog() is used for details often needed for debugging clangd sessions.
> // This level would typically be enabled for clangd developers.
> ```
My recent experience of debugging some weird GotoDef issues tells me that log 
of index should be on production (since it is a non-trivial part in a clangd 
session), it would be really helpful to understand what is going on. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+  Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);

JonasToth wrote:
> hugoeg wrote:
> > deannagarcia wrote:
> > > aaron.ballman wrote:
> > > > hokein wrote:
> > > > > aaron.ballman wrote:
> > > > > > I think this needs a `not(isExpansionInSystemHeader())` in there, 
> > > > > > as this is going to trigger on code that includes a header file 
> > > > > > using an `absl` namespace. If I'm incorrect and users typically 
> > > > > > include abseil as something other than system includes, you'll have 
> > > > > > to find a different way to solve this.
> > > > > Yeah, we have discussed this issue internally.  abseil-user code 
> > > > > usually includes abseil header like `#include 
> > > > > "whatever/abseil/base/optimization.h"`, so 
> > > > > `IsExpansionInSystemHeader` doesn't work for most cases. 
> > > > > 
> > > > > We need a way to filter out all headers being a part of abseil 
> > > > > library. I think we can create a matcher `InExpansionInAbseilHeader` 
> > > > > -- basically using `IsExpansionInFileMatching` with a regex 
> > > > > expression that matches typical abseil include path. What do you 
> > > > > think?
> > > > > 
> > > > > And we'll have more abseil checks (e.g.  `abseil-no-internal-deps`) 
> > > > > that use `InExpansionInAbseilHeader`, we should put it to a common 
> > > > > header.
> > > > I think that is a sensible approach.
> > > We will begin working on this, I think it will first be implemented in 
> > > abseil-no-internal-deps but once it looks good I will add it to this 
> > > patch.
> > I'm going to go ahead a implement this in ASTMatchers.h and include it on 
> > the patch for **abseil-no-internal-dep**s
> In principle it is ok, but I don't think ASTMatchers.h is the correct place, 
> because it is only of use to clang-tidy.
> 
> There is a `util` directory in clang-tidy for this kind of stuff. Defining 
> you own matchers works the same as in ASTMatchers, you can grep through 
> clang-tidy checks (e.g. `AST_MATCHER`) to have some examples of private 
> matchers.
`ASTMatchers.h` is not a reasonable place to put `asseil`-specific matchers. We 
 have `clang-tidy/utils/Matchers.h` for putting clang-tidy specific matchers. 
I'm not sure whether it is reasonable to put abseil-specific matchers there. 
Maybe we create a `AbseilMatcher.h` file in `clang-tidy/abseil` directory?


https://reviews.llvm.org/D50580



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

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

@hugoeg, it looks like you are waiting for review, but this patch doesn't 
include the `IsExpansionInAbseilHeader` matcher. What's your plan of it?




Comment at: test/clang-tidy/abseil-fake-declarations.h:1
+namespace std {
+struct string {

I'd expect this header file is used as as a real absl library file:

* create an `absl` directory in `test/clang-tidy/Inputs/`, and this directory 
is the abseil source root directory
* use the real absl function/class names instead of some fake names in the 
header, e.g. this file could be `test/clang-tidy/Inputs/absl/strings/str_cat.h`.

This would make the lit test align with the real world case, and it could be 
helpful if you implement the `IsExpansionInAbseilHeader` matcher in this check. 



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:19
+namespace absl {
+  void OpeningNamespace() {
+strings_internal::InternalFunction();

the style doesn't looks correct to me.


https://reviews.llvm.org/D50542



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

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

> Hmm, I think this is the same for other symbol payload e.g. definition can be 
> missing for a symbol. And it seems to me that the concern is on the 
> SymbolSlab level: if a slab is for a single TU, users should expect missing 
> information; if a slab is merged from all TUs, then users can expect 
> "complete" information. I think it's reasonable to assume that users of 
> SymbolSlab are aware of this. I think it's probably not worth the overhead of 
> maintaining and using two separate slabs.

My concerns of storing occurrences as an extra payload of `Symbol` are:

- `SymbolSlab` is more like an implementation detail.  Users of `SymbolIndex` 
are not aware of it, they only get `Symbol` objects, so it easily confuses 
users if they see any occurrence-related interface/member in `Symbol`. And we 
will write a looong comment explaining its correct behavior. It'd be better if 
we avoid this confusion in the API level.
- The fields in `Symbol` structure are symbol properties, and could be stored 
in memory. However, occurrences are not, we can't guarantee that.
- It seems that we are coupling `ID`, `Symbol`, `SymbolOccurrence` together: in 
the index implementation, we will go through ID=>Symbol=>Occurrences rather 
than ID=>Occurrences.

> I think these options are reasonable if they turn out to be necessary.

I think they are necessary. For collecting all occurrences for local symbols 
from the AST, we only need symbol occurrence information, other information 
(e.g. declaration&definition location, #include) should be discarded; Index for 
code completion should not collect symbol occurrences.

> And making the SymbolCollector more complicated doesn't seem to be a problem 
> if we are indeed doing more complicated work, but I don't think this would 
> turn into a big problem as logic of xrefs seems pretty isolated.

If xrefs is quite isolated, I think it is a good signal to have a dedicated 
class handling it.

> I think implementing xrefs in a separate class would likely to cause more 
> duplicate and maintenance, e.g. two sets of options, two sets of 
> initializations or life-time tracking of collectors (they look a lot alike), 
> the same boilerplate factory code in tests, passing around two collectors in 
> user code.

Merging xrefs to `SymbolCollector` couldn't avoid these problems, I think it is 
a matter of where we put these code:

- different initialization of `SymbolCollector` for different use cases (e.g. 
setting different flags in SymbolCollectorOptions).
- for dynamic index, index for xrefs and code completion would be triggered at 
different point: index for xrefs should happen when AST is ready; index for 
code completion happens when Preamble is ready;  we might end up with two slabs 
instances in the dynamic index (1 symbol slab + 1 occurrence slab vs. 2 symbol 
slabs).

The duplication is mainly about AST frontend action boilerplate code. To 
eliminate it, we could do some refactorings:

- get rid of the clang ast action code in SymbolCollector, and 
SymbolOccurrenceCollector
- introduce an `IndexSymbol` which is a subclass `index::IndexDataConsumer`
- the `IndexSymbol` has two mode (indexing symbol or indexing occurrence),  and 
dispatch ast information to `SymbolCollector`/`SymbolOccurrenceCollector`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:191
+void templated_thrower() { throw T{}(); }
+// CHECK-MESSAGES: [[@LINE-1]]:34: warning: throwing an exception whose type 
'int' is not derived from 'std::exception'
+

JonasToth wrote:
> JonasToth wrote:
> > JonasToth wrote:
> > > alexfh wrote:
> > > > hokein wrote:
> > > > > I think giving message on the template function here is confusing to 
> > > > > users even it gets instantiated somewhere in this TU -- because users 
> > > > > have to find the location that triggers the template instantiation.
> > > > > 
> > > > > Maybe 
> > > > > 1) Add a note which gives the instantiation location to the message, 
> > > > > or
> > > > > 2) ignore template case (some clang-tidy checks do this)
> > > > In this particular case it seems to be useful to get warnings from 
> > > > template instantiations. But the message will indeed be confusing.
> > > > 
> > > > Ideally, the message should have "in instantiation of xxx requested 
> > > > here" notes attached, as clang warnings do. But this is not working 
> > > > automatically, and it's implemented in Sema 
> > > > (`Sema::PrintInstantiationStack()` in 
> > > > lib/Sema/SemaTemplateInstantiate.cpp).
> > > > 
> > > > I wonder whether it's feasible to produce similar notes after Sema is 
> > > > dead? Maybe not the whole instantiation stack, but at least it should 
> > > > be possible to figure out that the enclosing function is a template 
> > > > instantiation or is a member function of an type that is an 
> > > > instantiation of a template. That would be useful for other checks as 
> > > > well.
> > > It should be possible to figure out, that the type comes from template 
> > > instantiation and that information could be added to the warning.
> > > 
> > > I will take a look at Sema and think about something like this. 
> > > Unfortunatly i dont have a lot of time :/
> > I did look further into the issue, i think it is non-trivial.
> > 
> > The newly added case is not a templated exception perse, but there is a 
> > exception-factory, which is templated, that creates a normal exception.
> > 
> > I did add another note for template instantiations, but i could not figure 
> > out a way to give better diagnostics for the new use-case.
> @hokein and @alexfh Do you still have your concerns (the exception is not a 
> template value, but the factory creating them) or is this fix acceptable?
I agree this is non-trivial. If we can't find a good solution at the moment, 
I'd prefer to ignore this case instead of adding some workarounds in the check, 
what do you think? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714



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

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 161176.
hokein marked an inline comment as done.
hokein added a comment.

Remove ASSERT_TRUE(bool(Preamble)).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50627

Files:
  unittests/clangd/TUSchedulerTests.cpp


Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -308,6 +308,51 @@
   UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
+TEST_F(TUSchedulerTests, EmptyPreamble) {
+  TUScheduler S(
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+  PreambleParsedCallback(),
+  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+  ASTRetentionPolicy());
+
+  auto Foo = testPath("foo.cpp");
+  auto Header = testPath("foo.h");
+
+  Files[Header] = "void foo()";
+  Timestamps[Header] = time_t(0);
+  auto WithPreamble = R"cpp(
+#include "foo.h"
+int main() {}
+  )cpp";
+  auto WithEmptyPreamble = R"cpp(int main() {})cpp";
+  S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  S.runWithPreamble("getNonEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get a non-empty preamble.
+  EXPECT_GT(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+  // Update the file which results in an empty preamble.
+  S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+  S.runWithPreamble("getEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get an empty preamble.
+  EXPECT_EQ(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+}
+
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.


Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -308,6 +308,51 @@
   UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
+TEST_F(TUSchedulerTests, EmptyPreamble) {
+  TUScheduler S(
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+  PreambleParsedCallback(),
+  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+  ASTRetentionPolicy());
+
+  auto Foo = testPath("foo.cpp");
+  auto Header = testPath("foo.h");
+
+  Files[Header] = "void foo()";
+  Timestamps[Header] = time_t(0);
+  auto WithPreamble = R"cpp(
+#include "foo.h"
+int main() {}
+  )cpp";
+  auto WithEmptyPreamble = R"cpp(int main() {})cpp";
+  S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  S.runWithPreamble("getNonEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get a non-empty preamble.
+  EXPECT_GT(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+  // Update the file which results in an empty preamble.
+  S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+  S.runWithPreamble("getEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get an empty preamble.
+  EXPECT_EQ(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+}
+
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/list

[PATCH] D50695: [clangd] Always use the latest preamble

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE340001: [clangd] Always use the latest preamble (authored 
by hokein, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50695?vs=160983&id=161177#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50695

Files:
  clangd/TUScheduler.cpp
  unittests/clangd/XRefsTests.cpp


Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -370,8 +370,7 @@
 bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
 {
   std::lock_guard Lock(Mutex);
-  if (NewPreamble)
-LastBuiltPreamble = NewPreamble;
+  LastBuiltPreamble = NewPreamble;
 }
 // Before doing the expensive AST reparse, we want to release our reference
 // to the old preamble, so it can be freed if there are no other references
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1021,6 +1021,51 @@
   EXPECT_THAT(*Locations, IsEmpty());
 }
 
+TEST(GoToDefinition, WithPreamble) {
+  // Test stragety: AST should always use the latest preamble instead of last
+  // good preamble.
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  auto FooCppUri = URIForFile{FooCpp};
+  // The trigger locations must be the same.
+  Annotations FooWithHeader(R"cpp(#include "fo^o.h")cpp");
+  Annotations FooWithoutHeader(R"cpp(double[[fo^o]]();)cpp");
+
+  FS.Files[FooCpp] = FooWithHeader.code();
+
+  auto FooH = testPath("foo.h");
+  auto FooHUri = URIForFile{FooH};
+  Annotations FooHeader(R"cpp([[]])cpp");
+  FS.Files[FooH] = FooHeader.code();
+
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // GoToDefinition goes to a #include file: the result comes from the 
preamble.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithHeader.point())),
+  ElementsAre(Location{FooHUri, FooHeader.range()}));
+
+  // Only preamble is built, and no AST is built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  // We build AST here, and it should use the latest preamble rather than the
+  // stale one.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+  ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+
+  // Reset test environment.
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // Both preamble and AST are built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  // Use the AST being built in above request.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+  ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -370,8 +370,7 @@
 bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
 {
   std::lock_guard Lock(Mutex);
-  if (NewPreamble)
-LastBuiltPreamble = NewPreamble;
+  LastBuiltPreamble = NewPreamble;
 }
 // Before doing the expensive AST reparse, we want to release our reference
 // to the old preamble, so it can be freed if there are no other references
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1021,6 +1021,51 @@
   EXPECT_THAT(*Locations, IsEmpty());
 }
 
+TEST(GoToDefinition, WithPreamble) {
+  // Test stragety: AST should always use the latest preamble instead of last
+  // good preamble.
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  auto FooCppUri = URIForFile{FooCpp};
+  // The trigger locations must be the same.
+  Annotations FooWithHeader(R"cpp(#include "fo^o.h")cpp");
+  Annotations FooWithoutHeader(R"cpp(double[[fo^o]]();)cpp");
+
+  FS.Files[FooCpp] = FooWithHeader.code();
+
+  auto FooH = testPath("foo.h");
+  auto FooHUri = URIForFile{FooH};
+  Annotations FooHeader(R"cpp([[]])cpp");
+  FS.Files[FooH] = FooHeader.code();
+
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // GoToDefinition goes to a #include file: the result comes from the preamble.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithHeader.point())),
+  ElementsAre(Location{FooH

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for adding it.




Comment at: clangd/TUScheduler.cpp:406
 // We only need to build the AST if diagnostics were requested.
 if (WantDiags == WantDiagnostics::No)
   return;

The AST might not get built if `WantDiags::No`, and will be built in 
`runWithAST`.



Comment at: clangd/TUScheduler.h:66
+  /// instead.
+  virtual void onMainAST(PathRef Path, ParsedAST &AST) = 0;
+};

Does the callback get called every time we built an AST? clangd only has 3 idle 
ASTs, if the AST is not there, we rebuild it when needed (even the source code 
is not changed), and we will update the dynamic index, which seems unnecessary.

It may rarely happen, 3 idle ASTs  might cover most cases, I think? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847



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

[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added reviewers: ilya-biryukov, ioeric.
Herald added subscribers: arphaman, jkorous, MaskRay.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50896

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/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -29,6 +29,7 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "hoverProvider": true,
+# CHECK-NEXT:  "referencesProvider": false,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -29,6 +29,7 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "hoverProvider": true,
+# CHECK-NEXT:  "referencesProvider": false,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: clangd/XRefs.h
===
--- clangd/XRefs.h
+++ clangd/XRefs.h
@@ -34,6 +34,11 @@
 /// Get the hover information when hovering at \p Pos.
 llvm::Optional getHover(ParsedAST &AST, Position Pos);
 
+/// Get references of symbol at a \p Pos.
+std::vector references(ParsedAST &AST, Position Pos,
+ bool includeDeclaration,
+ const SymbolIndex *Index = nullptr);
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -17,6 +17,7 @@
 #include "clang/Index/IndexingAction.h"
 #include "clang/Index/USRGeneration.h"
 #include "llvm/Support/Path.h"
+
 namespace clang {
 namespace clangd {
 using namespace llvm;
@@ -660,5 +661,12 @@
   return None;
 }
 
+std::vector references(ParsedAST &AST, Position Pos,
+ bool IncludeDeclaration,
+ const SymbolIndex *Index) {
+  // FIXME: implement it.
+  return {};
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -55,6 +55,7 @@
   virtual void onDocumentHighlight(TextDocumentPositionParams &Params) = 0;
   virtual void onHover(TextDocumentPositionParams &Params) = 0;
   virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
+  virtual void onReference(ReferenceParams &Params) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -75,4 +75,5 @@
   Register("workspace/didChangeConfiguration",
&ProtocolCallbacks::onChangeConfiguration);
   Register("workspace/symbol", &ProtocolCallbacks::onWorkspaceSymbol);
+  Register("textDocument/references", &ProtocolCallbacks::onReference);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -867,6 +867,17 @@
 llvm::json::Value toJSON(const DocumentHighlight &DH);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const DocumentHighlight &);
 
+struct ReferenceContext {
+  // Include the declaration of the current symbol.
+  bool includeDeclaration;
+};
+bool fromJSON(const llvm::json::Value &, ReferenceContext &);
+
+struct ReferenceParams : public TextDocumentPositionParams {
+  ReferenceContext context;
+};
+bool fromJSON(const llvm::json::Value &, ReferenceParams &);
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -615,5 +615,16 @@
  O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
 }
 
+bool fromJSON(const json::Value &Params, ReferenceContext &RC) {
+  json::ObjectMapper O(Params);
+  return O && O.map("includeDeclaration", RC.includeDeclaration);
+}
+
+bool fromJSON(const json::Value &Params, ReferenceParams &R) {
+  json::ObjectMapper O(Params);
+  return O && O.map("context", R.context) &&
+ O.map("textDocument", R.textDocument) && O.map("position", R.position);
+}
+
 } // name

[PATCH] D50862: [clang-tidy] Abseil: faster strsplit delimiter check

[Haojian Wu via Phabricator via cfe-commits]

hokein added a subscriber: sbenza.
hokein added a comment.

Thanks for porting the check to upstream (context: the check was developed 
internally, and has been run on our codebase for a while, it is pretty stable). 
Could you please update the patch message to indicate this is a porting change, 
and add the original check author @sbenza in the patch description?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50862



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

[PATCH] D50628: [Preamble] Empty preamble is not an error.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC340029: [Preamble] Empty preamble is not an error. (authored 
by hokein, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50628?vs=160319&id=161241#toc

Repository:
  rC Clang

https://reviews.llvm.org/D50628

Files:
  include/clang/Frontend/PrecompiledPreamble.h
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/PrecompiledPreamble.cpp


Index: lib/Frontend/ASTUnit.cpp
===
--- lib/Frontend/ASTUnit.cpp
+++ lib/Frontend/ASTUnit.cpp
@@ -1363,7 +1363,6 @@
 } else {
   switch (static_cast(NewPreamble.getError().value())) 
{
   case BuildPreambleError::CouldntCreateTempFile:
-  case BuildPreambleError::PreambleIsEmpty:
 // Try again next time.
 PreambleRebuildCounter = 1;
 return nullptr;
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -237,9 +237,6 @@
 PreambleCallbacks &Callbacks) {
   assert(VFS && "VFS is null");
 
-  if (!Bounds.Size)
-return BuildPreambleError::PreambleIsEmpty;
-
   auto PreambleInvocation = std::make_shared(Invocation);
   FrontendOptions &FrontendOpts = PreambleInvocation->getFrontendOpts();
   PreprocessorOptions &PreprocessorOpts =
@@ -423,9 +420,6 @@
   PreprocessorOptions &PreprocessorOpts =
   PreambleInvocation->getPreprocessorOpts();
 
-  if (!Bounds.Size)
-return false;
-
   // We've previously computed a preamble. Check whether we have the same
   // preamble now that we did before, and that there's enough space in
   // the main-file buffer within the precompiled preamble to fit the
@@ -758,8 +752,6 @@
 
 std::string BuildPreambleErrorCategory::message(int condition) const {
   switch (static_cast(condition)) {
-  case BuildPreambleError::PreambleIsEmpty:
-return "Preamble is empty";
   case BuildPreambleError::CouldntCreateTempFile:
 return "Could not create temporary file for PCH";
   case BuildPreambleError::CouldntCreateTargetInfo:
Index: include/clang/Frontend/PrecompiledPreamble.h
===
--- include/clang/Frontend/PrecompiledPreamble.h
+++ include/clang/Frontend/PrecompiledPreamble.h
@@ -286,8 +286,7 @@
 };
 
 enum class BuildPreambleError {
-  PreambleIsEmpty = 1,
-  CouldntCreateTempFile,
+  CouldntCreateTempFile = 1,
   CouldntCreateTargetInfo,
   BeginSourceFileFailed,
   CouldntEmitPCH


Index: lib/Frontend/ASTUnit.cpp
===
--- lib/Frontend/ASTUnit.cpp
+++ lib/Frontend/ASTUnit.cpp
@@ -1363,7 +1363,6 @@
 } else {
   switch (static_cast(NewPreamble.getError().value())) {
   case BuildPreambleError::CouldntCreateTempFile:
-  case BuildPreambleError::PreambleIsEmpty:
 // Try again next time.
 PreambleRebuildCounter = 1;
 return nullptr;
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -237,9 +237,6 @@
 PreambleCallbacks &Callbacks) {
   assert(VFS && "VFS is null");
 
-  if (!Bounds.Size)
-return BuildPreambleError::PreambleIsEmpty;
-
   auto PreambleInvocation = std::make_shared(Invocation);
   FrontendOptions &FrontendOpts = PreambleInvocation->getFrontendOpts();
   PreprocessorOptions &PreprocessorOpts =
@@ -423,9 +420,6 @@
   PreprocessorOptions &PreprocessorOpts =
   PreambleInvocation->getPreprocessorOpts();
 
-  if (!Bounds.Size)
-return false;
-
   // We've previously computed a preamble. Check whether we have the same
   // preamble now that we did before, and that there's enough space in
   // the main-file buffer within the precompiled preamble to fit the
@@ -758,8 +752,6 @@
 
 std::string BuildPreambleErrorCategory::message(int condition) const {
   switch (static_cast(condition)) {
-  case BuildPreambleError::PreambleIsEmpty:
-return "Preamble is empty";
   case BuildPreambleError::CouldntCreateTempFile:
 return "Could not create temporary file for PCH";
   case BuildPreambleError::CouldntCreateTargetInfo:
Index: include/clang/Frontend/PrecompiledPreamble.h
===
--- include/clang/Frontend/PrecompiledPreamble.h
+++ include/clang/Frontend/PrecompiledPreamble.h
@@ -286,8 +286,7 @@
 };
 
 enum class BuildPreambleError {
-  PreambleIsEmpty = 1,
-  CouldntCreateTempFile,
+  CouldntCreateTempFile = 1,
   CouldntCreateTargetInfo,
   BeginSourceFileFailed,
   CouldntEmitPCH
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20
+
+bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){
+  if (loc.isInvalid()) {

I think we can make it as an ASTMatcher instead of a function like:

```
AST_POLYMORPHIC_MATCHER_P(isInAbseilFile,
  AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, TypeLoc)) 
{
   // your code here.
}
```


https://reviews.llvm.org/D50542



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

[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/XRefs.cpp:665
+std::vector references(ParsedAST &AST, Position Pos,
+ bool IncludeDeclaration,
+ const SymbolIndex *Index) {

ilya-biryukov wrote:
> Are we going to support the `IncludeDeclaration` option?
> What should its semantics be?
> 
> I'm trying to figure out a better name for it, can't get what should it do by 
> looking at the code at the moment :-)
I think so. It is a term defined in LSP (although vscode always sets it 
`true`).  I think it aligns with the `clang::index::SymbolRole::Declaration`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50896



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

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340035: [clangd] Add a testcase for empty preamble. 
(authored by hokein, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D50627

Files:
  clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp


Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
@@ -308,6 +308,51 @@
   UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
+TEST_F(TUSchedulerTests, EmptyPreamble) {
+  TUScheduler S(
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+  PreambleParsedCallback(),
+  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+  ASTRetentionPolicy());
+
+  auto Foo = testPath("foo.cpp");
+  auto Header = testPath("foo.h");
+
+  Files[Header] = "void foo()";
+  Timestamps[Header] = time_t(0);
+  auto WithPreamble = R"cpp(
+#include "foo.h"
+int main() {}
+  )cpp";
+  auto WithEmptyPreamble = R"cpp(int main() {})cpp";
+  S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  S.runWithPreamble("getNonEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get a non-empty preamble.
+  EXPECT_GT(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+  // Update the file which results in an empty preamble.
+  S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+  S.runWithPreamble("getEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get an empty preamble.
+  EXPECT_EQ(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+}
+
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.


Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
@@ -308,6 +308,51 @@
   UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
+TEST_F(TUSchedulerTests, EmptyPreamble) {
+  TUScheduler S(
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+  PreambleParsedCallback(),
+  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+  ASTRetentionPolicy());
+
+  auto Foo = testPath("foo.cpp");
+  auto Header = testPath("foo.h");
+
+  Files[Header] = "void foo()";
+  Timestamps[Header] = time_t(0);
+  auto WithPreamble = R"cpp(
+#include "foo.h"
+int main() {}
+  )cpp";
+  auto WithEmptyPreamble = R"cpp(int main() {})cpp";
+  S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  S.runWithPreamble("getNonEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get a non-empty preamble.
+  EXPECT_GT(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+  // Update the file which results in an empty preamble.
+  S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+  S.runWithPreamble("getEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get an empty preamble.
+  EXPECT_EQ(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+}
+
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few p

[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check

[Haojian Wu via Phabricator via cfe-commits]

hokein closed this revision.
hokein added a comment.

Committed in https://reviews.llvm.org/rL340038.


https://reviews.llvm.org/D50389



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

[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D50896#1204310, @ilya-biryukov wrote:

> This change LG, but I would not commit it before we have an actual 
> implementation.
>  As soon as we have the `references` function in `ClangdUnit.cpp` 
> implemented, the merge of this change should be trivial.
>
> Is there any value in committing empty stubs before an actual implementation 
> is ready?


Actually, I'd prefer to get it submitted before we have the actual 
implementation -- because it would make use easier to experience xrefs features 
in LSP clients when adding xrefs actual implemenation (without patching the 
whole patch locally).

Is there any concern about it? Since we disable it by default, it would not 
affect any users.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50896



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

[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D50389#1204514, @Eugene.Zelenko wrote:

> Somehow documentation file was not committed.


Oops, I forgot to `git add` to the doc file. `arc patch` somehow failed to 
apply this patch, I applied it manually. Added in 
https://reviews.llvm.org/rL340075. Thanks for pointing it out.


https://reviews.llvm.org/D50389



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

[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added reviewers: ilya-biryukov, ioeric.
Herald added subscribers: kadircet, arphaman, mgrang, jkorous, MaskRay, 
javed.absar, mgorny.

Depends on the AST callback interfaces.

- https://reviews.llvm.org/D50847
- https://reviews.llvm.org/D50889


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50958

Files:
  clangd/CMakeLists.txt
  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/TUScheduler.cpp
  clangd/TUScheduler.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/Merge.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/CMakeLists.txt
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -17,10 +17,11 @@
 
 namespace clang {
 namespace clangd {
+namespace {
 
 using ::testing::_;
-using ::testing::Each;
 using ::testing::AnyOf;
+using ::testing::Each;
 using ::testing::Pair;
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
@@ -30,6 +31,18 @@
   handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
 }
 
+class NoopParsingCallbacks : public ParsingCallbacks {
+public:
+  static NoopParsingCallbacks& instance() {
+static NoopParsingCallbacks* Instance = new NoopParsingCallbacks;
+return *Instance;
+  }
+
+  void onPreambleAST(PathRef Path, ASTContext &Ctx,
+ std::shared_ptr PP) override {}
+  void onMainAST(PathRef Path, ParsedAST &AST) override {}
+};
+
 class TUSchedulerTests : public ::testing::Test {
 protected:
   ParseInputs getInputs(PathRef File, std::string Contents) {
@@ -45,7 +58,7 @@
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true,
-/*PreambleParsedCallback=*/nullptr,
+NoopParsingCallbacks::instance(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -102,7 +115,7 @@
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true,
-/*PreambleParsedCallback=*/nullptr,
+NoopParsingCallbacks::instance(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,11 +142,10 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true,
-  /*PreambleParsedCallback=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::seconds(1),
-  ASTRetentionPolicy());
+TUScheduler S(
+getDefaultAsyncThreadsCount(),
+/*StorePreamblesInMemory=*/true, NoopParsingCallbacks::instance(),
+/*UpdateDebounce=*/std::chrono::seconds(1), ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
 auto Path = testPath("foo.cpp");
 S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
@@ -163,7 +175,7 @@
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true,
-  /*PreambleParsedCallback=*/nullptr,
+  NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -261,7 +273,7 @@
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
   /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  PreambleParsedCallback(),
+  NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -358,7 +370,7 @@
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
   /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  PreambleParsedCallback(),
+  NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -391,7 +403,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, PreambleParsedCallback(),
+  /*StorePreambleInMemory=*/true, NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/st

[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 161441.
hokein added a comment.

More cleanups.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50958

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/TUScheduler.cpp
  clangd/TUScheduler.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/Merge.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -17,10 +17,11 @@
 
 namespace clang {
 namespace clangd {
+namespace {
 
 using ::testing::_;
-using ::testing::Each;
 using ::testing::AnyOf;
+using ::testing::Each;
 using ::testing::Pair;
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
@@ -30,6 +31,18 @@
   handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
 }
 
+class NoopParsingCallbacks : public ParsingCallbacks {
+public:
+  static NoopParsingCallbacks& instance() {
+static NoopParsingCallbacks* Instance = new NoopParsingCallbacks;
+return *Instance;
+  }
+
+  void onPreambleAST(PathRef Path, ASTContext &Ctx,
+ std::shared_ptr PP) override {}
+  void onMainAST(PathRef Path, ParsedAST &AST) override {}
+};
+
 class TUSchedulerTests : public ::testing::Test {
 protected:
   ParseInputs getInputs(PathRef File, std::string Contents) {
@@ -45,7 +58,7 @@
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true,
-/*PreambleParsedCallback=*/nullptr,
+NoopParsingCallbacks::instance(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -102,7 +115,7 @@
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true,
-/*PreambleParsedCallback=*/nullptr,
+NoopParsingCallbacks::instance(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,11 +142,10 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true,
-  /*PreambleParsedCallback=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::seconds(1),
-  ASTRetentionPolicy());
+TUScheduler S(
+getDefaultAsyncThreadsCount(),
+/*StorePreamblesInMemory=*/true, NoopParsingCallbacks::instance(),
+/*UpdateDebounce=*/std::chrono::seconds(1), ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
 auto Path = testPath("foo.cpp");
 S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
@@ -163,7 +175,7 @@
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true,
-  /*PreambleParsedCallback=*/nullptr,
+  NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -261,7 +273,7 @@
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
   /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  PreambleParsedCallback(),
+  NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -358,7 +370,7 @@
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
   /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  PreambleParsedCallback(),
+  NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -391,7 +403,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, PreambleParsedCallback(),
+  /*StorePreambleInMemory=*/true, NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -444,7 +456,7 @@
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true,

[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

This patch is incomplete, and it is a **prototype** based on our discussion 
last week. You can patch it locally, and play around with VSCode.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50958



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

[PATCH] D50960: [clangd] Add missing lock in the lookup.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ioeric.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ilya-biryukov.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50960

Files:
  clangd/index/MemIndex.cpp


Index: clangd/index/MemIndex.cpp
===
--- clangd/index/MemIndex.cpp
+++ clangd/index/MemIndex.cpp
@@ -64,6 +64,7 @@
 
 void MemIndex::lookup(const LookupRequest &Req,
   llvm::function_ref Callback) const 
{
+  std::lock_guard Lock(Mutex);
   for (const auto &ID : Req.IDs) {
 auto I = Index.find(ID);
 if (I != Index.end())


Index: clangd/index/MemIndex.cpp
===
--- clangd/index/MemIndex.cpp
+++ clangd/index/MemIndex.cpp
@@ -64,6 +64,7 @@
 
 void MemIndex::lookup(const LookupRequest &Req,
   llvm::function_ref Callback) const {
+  std::lock_guard Lock(Mutex);
   for (const auto &ID : Req.IDs) {
 auto I = Index.find(ID);
 if (I != Index.end())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50961: [clang-tidy] Add missing check documentation.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ioeric.
Herald added subscribers: kadircet, arphaman, jkorous, ilya-biryukov, xazax.hun.

[clangd] Simplify the code using UniqueStringSaver, NFC.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50961

Files:
  clangd/index/Index.cpp
  clangd/index/Index.h
  docs/clang-tidy/checks/abseil-duration-division.rst

Index: docs/clang-tidy/checks/abseil-duration-division.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-duration-division.rst
@@ -0,0 +1,36 @@
+.. title:: clang-tidy - abseil-duration-division
+
+abseil-duration-division
+
+
+``absl::Duration`` arithmetic works like it does with integers. That means that
+division of two ``absl::Duration`` objects returns an ``int64`` with any fractional
+component truncated toward 0. See `this link `_ for more information on arithmetic with ``absl::Duration``.
+
+For example:
+
+.. code-block:: c++
+
+ absl::Duration d = absl::Seconds(3.5);
+ int64 sec1 = d / absl::Seconds(1); // Truncates toward 0.
+ int64 sec2 = absl::ToInt64Seconds(d);  // Equivalent to division.
+ assert(sec1 == 3 && sec2 == 3);
+
+ double dsec = d / absl::Seconds(1);  // WRONG: Still truncates toward 0.
+ assert(dsec == 3.0);
+
+If you want floating-point division, you should use either the
+``absl::FDivDuration()`` function, or one of the unit conversion functions such
+as ``absl::ToDoubleSeconds()``. For example:
+
+.. code-block:: c++
+
+ absl::Duration d = absl::Seconds(3.5);
+ double dsec1 = absl::FDivDuration(d, absl::Seconds(1));  // GOOD: No truncation.
+ double dsec2 = absl::ToDoubleSeconds(d); // GOOD: No truncation.
+ assert(dsec1 == 3.5 && dsec2 == 3.5);
+
+
+This check looks for uses of ``absl::Duration`` division that is done in a
+floating-point context, and recommends the use of a function that returns a
+floating-point value.
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -17,6 +17,7 @@
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/StringSaver.h"
 #include 
 #include 
 
@@ -257,6 +258,8 @@
   // The frozen SymbolSlab will use less memory.
   class Builder {
   public:
+Builder() : UniqueStrings(Arena) {}
+
 // Adds a symbol, overwriting any existing one with the same ID.
 // This is a deep copy: underlying strings will be owned by the slab.
 void insert(const Symbol &S);
@@ -273,7 +276,7 @@
   private:
 llvm::BumpPtrAllocator Arena;
 // Intern table for strings. Contents are on the arena.
-llvm::DenseSet Strings;
+llvm::UniqueStringSaver UniqueStrings;
 std::vector Symbols;
 // Values are indices into Symbols vector.
 llvm::DenseMap SymbolIndex;
Index: clangd/index/Index.cpp
===
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -77,16 +77,10 @@
 }
 
 // Copy the underlying data of the symbol into the owned arena.
-static void own(Symbol &S, DenseSet &Strings,
+static void own(Symbol &S, llvm::UniqueStringSaver &Strings,
 BumpPtrAllocator &Arena) {
   // Intern replaces V with a reference to the same string owned by the arena.
-  auto Intern = [&](StringRef &V) {
-auto R = Strings.insert(V);
-if (R.second) { // New entry added to the table, copy the string.
-  *R.first = V.copy(Arena);
-}
-V = *R.first;
-  };
+  auto Intern = [&](StringRef &V) { V = Strings.save(V); };
 
   // We need to copy every StringRef field onto the arena.
   Intern(S.Name);
@@ -114,10 +108,10 @@
   auto R = SymbolIndex.try_emplace(S.ID, Symbols.size());
   if (R.second) {
 Symbols.push_back(S);
-own(Symbols.back(), Strings, Arena);
+own(Symbols.back(), UniqueStrings, Arena);
   } else {
 auto &Copy = Symbols[R.first->second] = S;
-own(Copy, Strings, Arena);
+own(Copy, UniqueStrings, Arena);
   }
 }
 
@@ -128,7 +122,7 @@
 [](const Symbol &L, const Symbol &R) { return L.ID < R.ID; });
   // We may have unused strings from overwritten symbols. Build a new arena.
   BumpPtrAllocator NewArena;
-  DenseSet Strings;
+  llvm::UniqueStringSaver Strings(NewArena);
   for (auto &S : Symbols)
 own(S, Strings, NewArena);
   return SymbolSlab(std::move(NewArena), std::move(Symbols));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50961: [clangd] Simplify the code using UniqueStringSaver, NFC.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 161445.
hokein added a comment.

Correct the patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50961

Files:
  clangd/index/Index.cpp
  clangd/index/Index.h
  docs/clang-tidy/checks/abseil-duration-division.rst

Index: docs/clang-tidy/checks/abseil-duration-division.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-duration-division.rst
@@ -0,0 +1,36 @@
+.. title:: clang-tidy - abseil-duration-division
+
+abseil-duration-division
+
+
+``absl::Duration`` arithmetic works like it does with integers. That means that
+division of two ``absl::Duration`` objects returns an ``int64`` with any fractional
+component truncated toward 0. See `this link `_ for more information on arithmetic with ``absl::Duration``.
+
+For example:
+
+.. code-block:: c++
+
+ absl::Duration d = absl::Seconds(3.5);
+ int64 sec1 = d / absl::Seconds(1); // Truncates toward 0.
+ int64 sec2 = absl::ToInt64Seconds(d);  // Equivalent to division.
+ assert(sec1 == 3 && sec2 == 3);
+
+ double dsec = d / absl::Seconds(1);  // WRONG: Still truncates toward 0.
+ assert(dsec == 3.0);
+
+If you want floating-point division, you should use either the
+``absl::FDivDuration()`` function, or one of the unit conversion functions such
+as ``absl::ToDoubleSeconds()``. For example:
+
+.. code-block:: c++
+
+ absl::Duration d = absl::Seconds(3.5);
+ double dsec1 = absl::FDivDuration(d, absl::Seconds(1));  // GOOD: No truncation.
+ double dsec2 = absl::ToDoubleSeconds(d); // GOOD: No truncation.
+ assert(dsec1 == 3.5 && dsec2 == 3.5);
+
+
+This check looks for uses of ``absl::Duration`` division that is done in a
+floating-point context, and recommends the use of a function that returns a
+floating-point value.
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -17,6 +17,7 @@
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/StringSaver.h"
 #include 
 #include 
 
@@ -257,6 +258,8 @@
   // The frozen SymbolSlab will use less memory.
   class Builder {
   public:
+Builder() : UniqueStrings(Arena) {}
+
 // Adds a symbol, overwriting any existing one with the same ID.
 // This is a deep copy: underlying strings will be owned by the slab.
 void insert(const Symbol &S);
@@ -273,7 +276,7 @@
   private:
 llvm::BumpPtrAllocator Arena;
 // Intern table for strings. Contents are on the arena.
-llvm::DenseSet Strings;
+llvm::UniqueStringSaver UniqueStrings;
 std::vector Symbols;
 // Values are indices into Symbols vector.
 llvm::DenseMap SymbolIndex;
Index: clangd/index/Index.cpp
===
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -77,16 +77,10 @@
 }
 
 // Copy the underlying data of the symbol into the owned arena.
-static void own(Symbol &S, DenseSet &Strings,
+static void own(Symbol &S, llvm::UniqueStringSaver &Strings,
 BumpPtrAllocator &Arena) {
   // Intern replaces V with a reference to the same string owned by the arena.
-  auto Intern = [&](StringRef &V) {
-auto R = Strings.insert(V);
-if (R.second) { // New entry added to the table, copy the string.
-  *R.first = V.copy(Arena);
-}
-V = *R.first;
-  };
+  auto Intern = [&](StringRef &V) { V = Strings.save(V); };
 
   // We need to copy every StringRef field onto the arena.
   Intern(S.Name);
@@ -114,10 +108,10 @@
   auto R = SymbolIndex.try_emplace(S.ID, Symbols.size());
   if (R.second) {
 Symbols.push_back(S);
-own(Symbols.back(), Strings, Arena);
+own(Symbols.back(), UniqueStrings, Arena);
   } else {
 auto &Copy = Symbols[R.first->second] = S;
-own(Copy, Strings, Arena);
+own(Copy, UniqueStrings, Arena);
   }
 }
 
@@ -128,7 +122,7 @@
 [](const Symbol &L, const Symbol &R) { return L.ID < R.ID; });
   // We may have unused strings from overwritten symbols. Build a new arena.
   BumpPtrAllocator NewArena;
-  DenseSet Strings;
+  llvm::UniqueStringSaver Strings(NewArena);
   for (auto &S : Symbols)
 own(S, Strings, NewArena);
   return SymbolSlab(std::move(NewArena), std::move(Symbols));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50961: [clangd] Simplify the code using UniqueStringSaver, NFC.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 161446.
hokein added a comment.

Update


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50961

Files:
  clangd/index/Index.cpp
  clangd/index/Index.h


Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -17,6 +17,7 @@
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/StringSaver.h"
 #include 
 #include 
 
@@ -257,6 +258,8 @@
   // The frozen SymbolSlab will use less memory.
   class Builder {
   public:
+Builder() : UniqueStrings(Arena) {}
+
 // Adds a symbol, overwriting any existing one with the same ID.
 // This is a deep copy: underlying strings will be owned by the slab.
 void insert(const Symbol &S);
@@ -273,7 +276,7 @@
   private:
 llvm::BumpPtrAllocator Arena;
 // Intern table for strings. Contents are on the arena.
-llvm::DenseSet Strings;
+llvm::UniqueStringSaver UniqueStrings;
 std::vector Symbols;
 // Values are indices into Symbols vector.
 llvm::DenseMap SymbolIndex;
Index: clangd/index/Index.cpp
===
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -77,16 +77,10 @@
 }
 
 // Copy the underlying data of the symbol into the owned arena.
-static void own(Symbol &S, DenseSet &Strings,
+static void own(Symbol &S, llvm::UniqueStringSaver &Strings,
 BumpPtrAllocator &Arena) {
   // Intern replaces V with a reference to the same string owned by the arena.
-  auto Intern = [&](StringRef &V) {
-auto R = Strings.insert(V);
-if (R.second) { // New entry added to the table, copy the string.
-  *R.first = V.copy(Arena);
-}
-V = *R.first;
-  };
+  auto Intern = [&](StringRef &V) { V = Strings.save(V); };
 
   // We need to copy every StringRef field onto the arena.
   Intern(S.Name);
@@ -114,10 +108,10 @@
   auto R = SymbolIndex.try_emplace(S.ID, Symbols.size());
   if (R.second) {
 Symbols.push_back(S);
-own(Symbols.back(), Strings, Arena);
+own(Symbols.back(), UniqueStrings, Arena);
   } else {
 auto &Copy = Symbols[R.first->second] = S;
-own(Copy, Strings, Arena);
+own(Copy, UniqueStrings, Arena);
   }
 }
 
@@ -128,7 +122,7 @@
 [](const Symbol &L, const Symbol &R) { return L.ID < R.ID; });
   // We may have unused strings from overwritten symbols. Build a new arena.
   BumpPtrAllocator NewArena;
-  DenseSet Strings;
+  llvm::UniqueStringSaver Strings(NewArena);
   for (auto &S : Symbols)
 own(S, Strings, NewArena);
   return SymbolSlab(std::move(NewArena), std::move(Symbols));


Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -17,6 +17,7 @@
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/StringSaver.h"
 #include 
 #include 
 
@@ -257,6 +258,8 @@
   // The frozen SymbolSlab will use less memory.
   class Builder {
   public:
+Builder() : UniqueStrings(Arena) {}
+
 // Adds a symbol, overwriting any existing one with the same ID.
 // This is a deep copy: underlying strings will be owned by the slab.
 void insert(const Symbol &S);
@@ -273,7 +276,7 @@
   private:
 llvm::BumpPtrAllocator Arena;
 // Intern table for strings. Contents are on the arena.
-llvm::DenseSet Strings;
+llvm::UniqueStringSaver UniqueStrings;
 std::vector Symbols;
 // Values are indices into Symbols vector.
 llvm::DenseMap SymbolIndex;
Index: clangd/index/Index.cpp
===
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -77,16 +77,10 @@
 }
 
 // Copy the underlying data of the symbol into the owned arena.
-static void own(Symbol &S, DenseSet &Strings,
+static void own(Symbol &S, llvm::UniqueStringSaver &Strings,
 BumpPtrAllocator &Arena) {
   // Intern replaces V with a reference to the same string owned by the arena.
-  auto Intern = [&](StringRef &V) {
-auto R = Strings.insert(V);
-if (R.second) { // New entry added to the table, copy the string.
-  *R.first = V.copy(Arena);
-}
-V = *R.first;
-  };
+  auto Intern = [&](StringRef &V) { V = Strings.save(V); };
 
   // We need to copy every StringRef field onto the arena.
   Intern(S.Name);
@@ -114,10 +108,10 @@
   auto R = SymbolIndex.try_emplace(S.ID, Symbols.size());
   if (R.second) {
 Symbols.push_back(S);
-own(Symbols.back(), Strings, Arena);
+own(Symbols.back(), UniqueStrings, Arena);
   } else {
 auto &Copy = Symbols[R.first->second] = S;
-own(Copy, Strings, Arena);
+own(Copy, UniqueStrings, Arena);
   }
 }
 
@@ -128,7 +122,7 @@
 [](const Symbol &L, const Symbol &R) { return L.ID < R.ID; });
   // We may have unused st

[PATCH] D50960: [clangd] Add missing lock in the lookup.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE340156: [clangd] Add missing lock in the lookup. (authored 
by hokein, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50960?vs=161442&id=161447#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50960

Files:
  clangd/index/MemIndex.cpp


Index: clangd/index/MemIndex.cpp
===
--- clangd/index/MemIndex.cpp
+++ clangd/index/MemIndex.cpp
@@ -64,6 +64,7 @@
 
 void MemIndex::lookup(const LookupRequest &Req,
   llvm::function_ref Callback) const 
{
+  std::lock_guard Lock(Mutex);
   for (const auto &ID : Req.IDs) {
 auto I = Index.find(ID);
 if (I != Index.end())


Index: clangd/index/MemIndex.cpp
===
--- clangd/index/MemIndex.cpp
+++ clangd/index/MemIndex.cpp
@@ -64,6 +64,7 @@
 
 void MemIndex::lookup(const LookupRequest &Req,
   llvm::function_ref Callback) const {
+  std::lock_guard Lock(Mutex);
   for (const auto &ID : Req.IDs) {
 auto I = Index.find(ID);
 if (I != Index.end())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

The interfaces look mostly good to me.




Comment at: clangd/ClangdServer.cpp:73
+
+  void onPreambleAST(PathRef Path, ASTContext &Ctx,
+ std::shared_ptr PP) override {

I think `Ctx` should be a `pointer` which gives us a way (passing a nullptr) to 
clean up the `FileIndex`, the same as `ParsedAST` below.

And I discover that we don't cleanup dynamic index when a file is being closed, 
is it expected or a bug?



Comment at: clangd/TUScheduler.cpp:406
 // We only need to build the AST if diagnostics were requested.
 if (WantDiags == WantDiagnostics::No)
   return;

ilya-biryukov wrote:
> hokein wrote:
> > The AST might not get built if `WantDiags::No`, and will be built in 
> > `runWithAST`.
> I suggest we keep it a known limitation and not rebuild the index in that 
> case.
> The original intent of `WantDiags::No` was to update the contents of the 
> file, so that upcoming request can see the new contents (e.g. this is a hack 
> to avoid passing overriden contents of the file in the request itself).
> `WantDiags::No` is always followed by an actual request that wants the 
> diagnostics (and will, therefore, build the AST and emit the callback).
> 
> Alternatively, we could unify the code paths building the AST. Would be a bit 
> more intrusive change, but I guess it's worth it.
I see, SG, thanks!



Comment at: clangd/TUScheduler.h:66
+  /// instead.
+  virtual void onMainAST(PathRef Path, ParsedAST &AST) = 0;
+};

ioeric wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > Does the callback get called every time we built an AST? clangd only has 
> > > 3 idle ASTs, if the AST is not there, we rebuild it when needed (even the 
> > > source code is not changed), and we will update the dynamic index, which 
> > > seems unnecessary.
> > > 
> > > It may rarely happen, 3 idle ASTs  might cover most cases, I think? 
> > Currently we only call this when AST is built for diagnostics, so we will 
> > have only a single callback, even if the AST will be rebuilt multiple times 
> > because it was flushed out of the cash (as long as the file contents didn't 
> > change, of course).
> > 
> > So we do behave optimally in that case and I suggest we keep it this way
> is there any overlap between preamble AST and main AST? If so, could you 
> elaborate in the documentation? 
> Currently we only call this when AST is built for diagnostics, so we will 
> have only a single callback, even if the AST will be rebuilt multiple times 
> because it was flushed out of the cash (as long as the file contents didn't 
> change, of course).

It sounds reasonable, thanks for the explanation. Could you clarify it in the 
comment?

> is there any overlap between preamble AST and main AST? If so, could you 
> elaborate in the documentation?

AFAIK, both of them contain different `TopLevelDecls`, but it is still possible 
to get the `Decl*` (declared in header) when visiting the main AST (e.g. when 
visiting the `void foo() {}` definition in `MainAST`, we can still get the 
`void foo();` declaration in `PreambleAST`). 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847



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

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.
Herald added a subscriber: kadircet.



Comment at: clangd/index/FileIndex.h:70
+  void
+  update(PathRef Path, ASTContext *AST, std::shared_ptr PP,
+ llvm::Optional> TopLevelDecls = llvm::None);

Any strong reason to unify the interface (and `indexAST`)? 

It seems to me that we do different things on `PreambleAST` and `MainAST`:

- on `PreambleAST`, we only index `Symbols` for code completion.
- on `MainAST`, we index `Symbols` (to collect other information missing from 
`PreambleAST`, e.g. definition location), and collect symbol references.

Therefore, we'd need to set different options for `SymbolCollector` 
accordingly. Having a single interface would make the implementation tricky 
(checking `TopLevelDecls` to see whether this is a `PreambleASt`).  


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889



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

[PATCH] D50961: [clangd] Simplify the code using UniqueStringSaver, NFC.

[Haojian Wu via Phabricator via cfe-commits]

hokein closed this revision.
hokein added a comment.

committed in https://reviews.llvm.org/rL340161.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50961



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

[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

I think it is reasonable to make Sema support suggesting override methods, 
instead of implementing it in clangd side?




Comment at: clangd/CodeComplete.cpp:206
+  llvm::StringMap> Overrides;
+  for (auto *Method : dyn_cast(DC)->methods()) {
+if (!Method->isVirtual())

nit: CR->methods().



Comment at: clangd/CodeComplete.cpp:210
+const std::string Name = Method->getNameAsString();
+const auto it = Overrides.find(Name);
+if (it == Overrides.end())

nit: we can simplify the code like `Overrides[Name].push_back(Method)`.



Comment at: clangd/CodeComplete.cpp:219
+  for (const auto &Base : CR->bases()) {
+const auto *BR = Base.getType().getTypePtr()->getAsCXXRecordDecl();
+for (auto *Method : BR->methods()) {

I think we should check whether `BR == nullptr` here.



Comment at: clangd/CodeComplete.cpp:1233
+// struct/class/union definition.
+const auto Overrides = getVirtualNonOverridenMethods(
+Recorder->CCSema->CurContext, Recorder->CCSema);

It seems that we treat it as a special case, the code path here runs out of the 
`ranking` path.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50898



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

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ioeric.
Herald added a subscriber: llvm-commits.

Passing a nullptr to memcpy is UB.


Repository:
  rL LLVM

https://reviews.llvm.org/D50966

Files:
  lib/Support/StringSaver.cpp


Index: lib/Support/StringSaver.cpp
===
--- lib/Support/StringSaver.cpp
+++ lib/Support/StringSaver.cpp
@@ -12,6 +12,8 @@
 using namespace llvm;
 
 StringRef StringSaver::save(StringRef S) {
+  if (S.empty())
+return StringRef();
   char *P = Alloc.Allocate(S.size() + 1);
   memcpy(P, S.data(), S.size());
   P[S.size()] = '\0';


Index: lib/Support/StringSaver.cpp
===
--- lib/Support/StringSaver.cpp
+++ lib/Support/StringSaver.cpp
@@ -12,6 +12,8 @@
 using namespace llvm;
 
 StringRef StringSaver::save(StringRef S) {
+  if (S.empty())
+return StringRef();
   char *P = Alloc.Allocate(S.size() + 1);
   memcpy(P, S.data(), S.size());
   P[S.size()] = '\0';
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50967: [Preamble] Fix an undefined behavior when checking an empty preamble can be reused.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.

Passing nullptr to memcmp is UB.


Repository:
  rC Clang

https://reviews.llvm.org/D50967

Files:
  lib/Frontend/PrecompiledPreamble.cpp


Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -416,6 +416,9 @@
   Bounds.Size <= MainFileBuffer->getBufferSize() &&
   "Buffer is too large. Bounds were calculated from a different buffer?");
 
+  if (Bounds.Size == 0)
+return PreambleBytes.size() == Bounds.Size;
+
   auto PreambleInvocation = std::make_shared(Invocation);
   PreprocessorOptions &PreprocessorOpts =
   PreambleInvocation->getPreprocessorOpts();


Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -416,6 +416,9 @@
   Bounds.Size <= MainFileBuffer->getBufferSize() &&
   "Buffer is too large. Bounds were calculated from a different buffer?");
 
+  if (Bounds.Size == 0)
+return PreambleBytes.size() == Bounds.Size;
+
   auto PreambleInvocation = std::make_shared(Invocation);
   PreprocessorOptions &PreprocessorOpts =
   PreambleInvocation->getPreprocessorOpts();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 161467.
hokein added a comment.

Correct the fix.


Repository:
  rL LLVM

https://reviews.llvm.org/D50966

Files:
  lib/Support/StringSaver.cpp


Index: lib/Support/StringSaver.cpp
===
--- lib/Support/StringSaver.cpp
+++ lib/Support/StringSaver.cpp
@@ -13,7 +13,8 @@
 
 StringRef StringSaver::save(StringRef S) {
   char *P = Alloc.Allocate(S.size() + 1);
-  memcpy(P, S.data(), S.size());
+  if (!S.empty())
+memcpy(P, S.data(), S.size());
   P[S.size()] = '\0';
   return StringRef(P, S.size());
 }


Index: lib/Support/StringSaver.cpp
===
--- lib/Support/StringSaver.cpp
+++ lib/Support/StringSaver.cpp
@@ -13,7 +13,8 @@
 
 StringRef StringSaver::save(StringRef S) {
   char *P = Alloc.Allocate(S.size() + 1);
-  memcpy(P, S.data(), S.size());
+  if (!S.empty())
+memcpy(P, S.data(), S.size());
   P[S.size()] = '\0';
   return StringRef(P, S.size());
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: lib/Support/StringSaver.cpp:14
 
 StringRef StringSaver::save(StringRef S) {
+  if (S.empty())

ioeric wrote:
> Is it possible to reuse `StringRef::copy(Allocator &)` here?
Unfortunately, not, `StringRef::copy` will change the behavior. The `StringRef` 
returned here is null-terminated.


Repository:
  rL LLVM

https://reviews.llvm.org/D50966



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

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340170: Fix an undefined behavior when storing an empty 
StringRef. (authored by hokein, committed by ).

Repository:
  rL LLVM

https://reviews.llvm.org/D50966

Files:
  llvm/trunk/lib/Support/StringSaver.cpp


Index: llvm/trunk/lib/Support/StringSaver.cpp
===
--- llvm/trunk/lib/Support/StringSaver.cpp
+++ llvm/trunk/lib/Support/StringSaver.cpp
@@ -13,7 +13,8 @@
 
 StringRef StringSaver::save(StringRef S) {
   char *P = Alloc.Allocate(S.size() + 1);
-  memcpy(P, S.data(), S.size());
+  if (!S.empty())
+memcpy(P, S.data(), S.size());
   P[S.size()] = '\0';
   return StringRef(P, S.size());
 }


Index: llvm/trunk/lib/Support/StringSaver.cpp
===
--- llvm/trunk/lib/Support/StringSaver.cpp
+++ llvm/trunk/lib/Support/StringSaver.cpp
@@ -13,7 +13,8 @@
 
 StringRef StringSaver::save(StringRef S) {
   char *P = Alloc.Allocate(S.size() + 1);
-  memcpy(P, S.data(), S.size());
+  if (!S.empty())
+memcpy(P, S.data(), S.size());
   P[S.size()] = '\0';
   return StringRef(P, S.size());
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20
+
+bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){
+  if (loc.isInvalid()) {

hugoeg wrote:
> hokein wrote:
> > I think we can make it as an ASTMatcher instead of a function like:
> > 
> > ```
> > AST_POLYMORPHIC_MATCHER_P(isInAbseilFile,
> >   AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, 
> > TypeLoc)) {
> >// your code here.
> > }
> > ```
> Unfortunately, I do not think we can.  That was the way I originally tried to 
> implement it. It worked on no-namespace-check, but not in this one. This is 
> because as an AST_POLYMORPHIC_MATCHER_P, we are trying to match an usage of a 
> Decl, not the Decl itself and since we are matching a TypeLoc in 
> no-internal-deps-check, not really the usage, it doesn't work.
> 
> As a result, I modified my original implementation, which you will see in 
> no-namespace-check and turned it into IsInAbseilFile(SourceManager&,  
> SourceLocation), which is just takes a source location and returns whether 
> the location we matched is in an Abseil file
Could you explain a bit more why it won't work? I don't understand why it 
doesn't work. 

Basically you define the matcher like:

```
AST_POLYMORPHIC_MATCHER(
isInAbseilFile, AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, TypeLoc,
NestedNameSpecifierLoc)) {
  auto &SourceManager = Finder->getASTContext().getSourceManager();
  SourceLocation loc = Node.getBeginLoc();
  if (loc.isInvalid()) return false;
  const FileEntry *FileEntry =
  SourceManager.getFileEntryForID(SourceManager.getFileID(loc));
  if (!FileEntry) return false;
  StringRef Filename = FileEntry->getName();
  llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
 "synchronization|types|utiliy)");
  return RE.match(Filename);
}
```

And use it for example in your check

```
Finder->addMatcher(nestedNameSpecifierLoc(
 loc(specifiesNamespace(namespaceDecl(
 matchesName("internal"),
 hasParent(namespaceDecl(hasName("absl")),
 unless(isInAbseilFile()))
 .bind("InternalDep"),
 this);
```



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20
+
+bool IsInAbseilFile(const SourceManager &manager, SourceLocation loc) {
+  if (loc.isInvalid()) return false;

nit: LLVM code guideline uses `Camel` for variable names.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:26
+  llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");
+  return RE.match(Filename);

typo: utiliy => utility.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:51
+  InternalDependency->getBeginLoc())) {
+
+diag(InternalDependency->getBeginLoc(),

nit: remove the empty line.



Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:8
+is in a namespace that includes the word “internal”, code is not allowed to 
+depend upon it beaucse it’s an implementation detail. They cannot friend it, 
+include it, you mention it or refer to it in any way. Doing so violates 

This sentence doesn't parse.



Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:17
+
+  absl::strings_internal::foo();
+  class foo{

nit: Please add a trailing comment on the line where it'd trigger the warning.



Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:18
+  absl::strings_internal::foo();
+  class foo{
+friend struct absl::container_internal::faa;

nit:  space between `foo` and `{`.


https://reviews.llvm.org/D50542



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

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/abseil/AbseilMatcher.h:16
+
+/// Matches AST nodes that were expanded within abseil-header-files.
+AST_POLYMORPHIC_MATCHER(isExpansionNotInAbseilHeader,

nit: We need proper documentation for this matcher, since it is exposed to 
users.



Comment at: clang-tidy/abseil/AbseilMatcher.h:17
+/// Matches AST nodes that were expanded within abseil-header-files.
+AST_POLYMORPHIC_MATCHER(isExpansionNotInAbseilHeader,
+AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, TypeLoc,

- using negative for AST matcher seems uncommon in ASTMatchers, for negative we 
have `unless`, so I'd suggest implementing `isInAbseilHeader`.
- it seems that this matcher is very similar to the matcher in 
https://reviews.llvm.org/D50542.  I think `isInAbseilFile` should also cover 
your case here, and also ignore the warning if you run the check on abseil core 
files.



Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+  auto Filename = FileEntry->getName();
+  llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");

The regex seems incomplete, we are missing `algorithm`, `time` subdirectory. 



Comment at: clang-tidy/abseil/AbseilMatcher.h:33
+  llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");
+  return (!RE.match(Filename));

typo: `utility`



Comment at: clang-tidy/abseil/AbseilMatcher.h:34
+ "synchronization|types|utiliy)");
+  return (!RE.match(Filename));
+}

nit: remove the outer `()`. 


https://reviews.llvm.org/D50580



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

[PATCH] D50862: [clang-tidy] Abseil: faster strsplit delimiter check

[Haojian Wu via Phabricator via cfe-commits]

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

The patch looks good. I'll commit for you once @JonasToth has no further 
comments.


https://reviews.llvm.org/D50862



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

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+  auto Filename = FileEntry->getName();
+  llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");

lebedev.ri wrote:
> hokein wrote:
> > The regex seems incomplete, we are missing `algorithm`, `time` 
> > subdirectory. 
> So what happens if open the namespace in a file that is located in my 
> personal `absl/base` directory?
> It will be false-negatively not reported?
 Yes, I'd say this is a known limitation.


https://reviews.llvm.org/D50580



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

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Haojian Wu via Phabricator via cfe-commits]

hokein added a subscriber: deannagarcia.
hokein added a comment.

Your patch seems have some comments unaddressed but you marked done.

About the abseil matcher stuff, you and @deannagarcia have an overlap, maybe 
just wait for https://reviews.llvm.org/D50580 to submit, and reuse the matcher 
in this patch?




Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+  auto &SourceManager = Finder->getASTContext().getSourceManager();
+  SourceLocation loc = Node.getBeginLoc();
+  if (loc.isInvalid())

nit: loc => Loc


https://reviews.llvm.org/D50542



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

[PATCH] D51041: [clang-tidy] Don't run misc-unused-using-decls check in C++17.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added a subscriber: xazax.hun.

It introduces some false positives which are non-trivial to fix.
Ignore running the check in C++17.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51041

Files:
  clang-tidy/misc/UnusedUsingDeclsCheck.cpp
  test/clang-tidy/misc-unused-using-decls-cxx17.cpp


Index: test/clang-tidy/misc-unused-using-decls-cxx17.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-unused-using-decls-cxx17.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -- 
-fno-delayed-template-parsing -std=gnu++17
+
+namespace ns {
+template 
+class KV {
+public:
+  KV(K, V);
+};
+}
+
+using ns::KV;
+
+void f() {
+  KV(1, 2);
+}
Index: clang-tidy/misc/UnusedUsingDeclsCheck.cpp
===
--- clang-tidy/misc/UnusedUsingDeclsCheck.cpp
+++ clang-tidy/misc/UnusedUsingDeclsCheck.cpp
@@ -29,6 +29,12 @@
 }
 
 void UnusedUsingDeclsCheck::registerMatchers(MatchFinder *Finder) {
+  // FIXME: make the check support C++17 or later. The check relies on the fact
+  // that the used declarations are visited after the "using" declaration, but
+  // it is not ture in C++17's template argument deduction.
+  if (!getLangOpts().CPlusPlus || getLangOpts().CPlusPlus17 ||
+  getLangOpts().CPlusPlus2a)
+return;
   Finder->addMatcher(usingDecl(isExpansionInMainFile()).bind("using"), this);
   auto DeclMatcher = hasDeclaration(namedDecl().bind("used"));
   Finder->addMatcher(loc(enumType(DeclMatcher)), this);


Index: test/clang-tidy/misc-unused-using-decls-cxx17.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-unused-using-decls-cxx17.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -- -fno-delayed-template-parsing -std=gnu++17
+
+namespace ns {
+template 
+class KV {
+public:
+  KV(K, V);
+};
+}
+
+using ns::KV;
+
+void f() {
+  KV(1, 2);
+}
Index: clang-tidy/misc/UnusedUsingDeclsCheck.cpp
===
--- clang-tidy/misc/UnusedUsingDeclsCheck.cpp
+++ clang-tidy/misc/UnusedUsingDeclsCheck.cpp
@@ -29,6 +29,12 @@
 }
 
 void UnusedUsingDeclsCheck::registerMatchers(MatchFinder *Finder) {
+  // FIXME: make the check support C++17 or later. The check relies on the fact
+  // that the used declarations are visited after the "using" declaration, but
+  // it is not ture in C++17's template argument deduction.
+  if (!getLangOpts().CPlusPlus || getLangOpts().CPlusPlus17 ||
+  getLangOpts().CPlusPlus2a)
+return;
   Finder->addMatcher(usingDecl(isExpansionInMainFile()).bind("using"), this);
   auto DeclMatcher = hasDeclaration(namedDecl().bind("used"));
   Finder->addMatcher(loc(enumType(DeclMatcher)), this);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the updates. Looks mostly good, a few nits.




Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+
+AST_POLYMORPHIC_MATCHER(isExpansionInAbseilHeader,
+AST_POLYMORPHIC_SUPPORTED_TYPES(

nit: this matcher name now should be `isInAbseilFile`.



Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:26
+  Finder->addMatcher(
+  namespaceDecl(hasName("absl"), unless(isExpansionInAbseilHeader()))
+  .bind("abslNamespace"),

Does the `absl` namespace is always the top level namespace? If so, I think it 
would be safer to use qualified name "::absl" here.



Comment at: clang-tidy/abseil/NoNamespaceCheck.h:19
+
+/// This test ensures users don't open namespace absl, as that violates
+/// our compatibility guidelines.

nit: test => check.



Comment at: clang-tidy/abseil/NoNamespaceCheck.h:20
+/// This test ensures users don't open namespace absl, as that violates
+/// our compatibility guidelines.
+///

nit: our = abseil?



Comment at: test/clang-tidy/abseil-no-namespace.cpp:22
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: namespace 'absl' 
+

nit: put it immediately after the above `name absl {`.


https://reviews.llvm.org/D50580



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

[PATCH] D50967: [Preamble] Fix an undefined behavior when checking an empty preamble can be reused.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 161891.
hokein added a comment.

Use the std::equal to replace the memcmp.


Repository:
  rC Clang

https://reviews.llvm.org/D50967

Files:
  lib/Frontend/PrecompiledPreamble.cpp


Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -426,8 +426,8 @@
   // new main file.
   if (PreambleBytes.size() != Bounds.Size ||
   PreambleEndsAtStartOfLine != Bounds.PreambleEndsAtStartOfLine ||
-  memcmp(PreambleBytes.data(), MainFileBuffer->getBufferStart(),
- Bounds.Size) != 0)
+  !std::equal(PreambleBytes.begin(), PreambleBytes.end(),
+  MainFileBuffer->getBuffer().begin()))
 return false;
   // The preamble has not changed. We may be able to re-use the precompiled
   // preamble.


Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -426,8 +426,8 @@
   // new main file.
   if (PreambleBytes.size() != Bounds.Size ||
   PreambleEndsAtStartOfLine != Bounds.PreambleEndsAtStartOfLine ||
-  memcmp(PreambleBytes.data(), MainFileBuffer->getBufferStart(),
- Bounds.Size) != 0)
+  !std::equal(PreambleBytes.begin(), PreambleBytes.end(),
+  MainFileBuffer->getBuffer().begin()))
 return false;
   // The preamble has not changed. We may be able to re-use the precompiled
   // preamble.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51041: [clang-tidy] Don't run misc-unused-using-decls check in C++17.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 161893.
hokein added a comment.

Remove redundant CPlusPlus2a.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51041

Files:
  clang-tidy/misc/UnusedUsingDeclsCheck.cpp
  test/clang-tidy/misc-unused-using-decls-cxx17.cpp


Index: test/clang-tidy/misc-unused-using-decls-cxx17.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-unused-using-decls-cxx17.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -- 
-fno-delayed-template-parsing -std=gnu++17
+
+namespace ns {
+template 
+class KV {
+public:
+  KV(K, V);
+};
+}
+
+using ns::KV;
+
+void f() {
+  KV(1, 2);
+}
Index: clang-tidy/misc/UnusedUsingDeclsCheck.cpp
===
--- clang-tidy/misc/UnusedUsingDeclsCheck.cpp
+++ clang-tidy/misc/UnusedUsingDeclsCheck.cpp
@@ -29,6 +29,11 @@
 }
 
 void UnusedUsingDeclsCheck::registerMatchers(MatchFinder *Finder) {
+  // FIXME: make the check support C++17 or later. The check relies on the fact
+  // that the used declarations are visited after the "using" declaration, but
+  // it is not ture in C++17's template argument deduction.
+  if (!getLangOpts().CPlusPlus || getLangOpts().CPlusPlus17)
+return;
   Finder->addMatcher(usingDecl(isExpansionInMainFile()).bind("using"), this);
   auto DeclMatcher = hasDeclaration(namedDecl().bind("used"));
   Finder->addMatcher(loc(enumType(DeclMatcher)), this);


Index: test/clang-tidy/misc-unused-using-decls-cxx17.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-unused-using-decls-cxx17.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -- -fno-delayed-template-parsing -std=gnu++17
+
+namespace ns {
+template 
+class KV {
+public:
+  KV(K, V);
+};
+}
+
+using ns::KV;
+
+void f() {
+  KV(1, 2);
+}
Index: clang-tidy/misc/UnusedUsingDeclsCheck.cpp
===
--- clang-tidy/misc/UnusedUsingDeclsCheck.cpp
+++ clang-tidy/misc/UnusedUsingDeclsCheck.cpp
@@ -29,6 +29,11 @@
 }
 
 void UnusedUsingDeclsCheck::registerMatchers(MatchFinder *Finder) {
+  // FIXME: make the check support C++17 or later. The check relies on the fact
+  // that the used declarations are visited after the "using" declaration, but
+  // it is not ture in C++17's template argument deduction.
+  if (!getLangOpts().CPlusPlus || getLangOpts().CPlusPlus17)
+return;
   Finder->addMatcher(usingDecl(isExpansionInMainFile()).bind("using"), this);
   auto DeclMatcher = hasDeclaration(namedDecl().bind("used"));
   Finder->addMatcher(loc(enumType(DeclMatcher)), this);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51041: [clang-tidy] Don't run misc-unused-using-decls check in C++17.

[Haojian Wu via Phabricator via cfe-commits]

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



Comment at: clang-tidy/misc/UnusedUsingDeclsCheck.cpp:35
+  // it is not ture in C++17's template argument deduction.
+  if (!getLangOpts().CPlusPlus || getLangOpts().CPlusPlus17 ||
+  getLangOpts().CPlusPlus2a)

xazax.hun wrote:
> Isn't the check for `getLangOpts().CPlusPlus2a` redundant? Shouldn't it imply 
> the C++17 flag?
Didn't notice it. Yeah, you are right. If `CPlusCplus2a` also indicates C++17.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51041



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

[PATCH] D51061: [clang-tidy] abseil-str-cat-append

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Please make sure the code follows the LLVM code style, e.g. the variable name 
format "CamelName".


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51061



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

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Haojian Wu via Phabricator via cfe-commits]

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

Thanks! Looks good.




Comment at: clangd/ClangdServer.cpp:73
+
+  void onPreambleAST(PathRef Path, ASTContext &Ctx,
+ std::shared_ptr PP) override {

ilya-biryukov wrote:
> hokein wrote:
> > I think `Ctx` should be a `pointer` which gives us a way (passing a 
> > nullptr) to clean up the `FileIndex`, the same as `ParsedAST` below.
> > 
> > And I discover that we don't cleanup dynamic index when a file is being 
> > closed, is it expected or a bug?
> I suggest we add an extra method to `DynamicIndex` that we call when the file 
> is closed. I don't think it's intentional that we don't clean up the index 
> for the closed files.
> Not sure what's the best way to handle invalid ASTs, but we're never calling 
> the current callback with `nullptr` anyway, so I suggest we keep it a 
> reference to better reflect that callbacks don't actually handle any errors 
> for us.
SG, and it is out of scope of this patch. Let's figure it out in the 
clean-up-index patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847



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

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 161927.
hokein added a comment.
Herald added a subscriber: kadircet.

Update the patch based on our offline discussion

- only one single clang intefaces implementation, and move finding references 
to current symbol collector;
- store references in SymbolSlab;


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385

Files:
  clangd/index/FileIndex.cpp
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -74,6 +74,14 @@
 MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
   return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
 }
+MATCHER(OccurrenceRange, "") {
+  const clang::clangd::SymbolOccurrence &Pos = testing::get<0>(arg);
+  const clang::clangd::Range &Range = testing::get<1>(arg);
+  return std::tie(Pos.Location.Start.Line, Pos.Location.Start.Column,
+  Pos.Location.End.Line, Pos.Location.End.Column) ==
+ std::tie(Range.start.line, Range.start.character, Range.end.line,
+  Range.end.character);
+}
 
 namespace clang {
 namespace clangd {
@@ -95,7 +103,7 @@
 assert(AST.hasValue());
 return SymbolCollector::shouldCollectSymbol(
 Qualified ? findDecl(*AST, Name) : findAnyDecl(*AST, Name),
-AST->getASTContext(), SymbolCollector::Options());
+AST->getASTContext());
   }
 
 protected:
@@ -162,8 +170,10 @@
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
   SymbolIndexActionFactory(SymbolCollector::Options COpts,
-   CommentHandler *PragmaHandler)
-  : COpts(std::move(COpts)), PragmaHandler(PragmaHandler) {}
+   CommentHandler *PragmaHandler,
+   index::IndexingOptions IndexOpts)
+  : COpts(std::move(COpts)), PragmaHandler(PragmaHandler),
+IndexOpts(IndexOpts) {}
 
   clang::FrontendAction *create() override {
 class WrappedIndexAction : public WrapperFrontendAction {
@@ -186,18 +196,16 @@
   index::IndexingOptions IndexOpts;
   CommentHandler *PragmaHandler;
 };
-index::IndexingOptions IndexOpts;
-IndexOpts.SystemSymbolFilter =
-index::IndexingOptions::SystemSymbolFilterKind::All;
-IndexOpts.IndexFunctionLocals = false;
 Collector = std::make_shared(COpts);
 return new WrappedIndexAction(Collector, std::move(IndexOpts),
   PragmaHandler);
   }
 
   std::shared_ptr Collector;
   SymbolCollector::Options COpts;
   CommentHandler *PragmaHandler;
+
+  index::IndexingOptions IndexOpts;
 };
 
 class SymbolCollectorTest : public ::testing::Test {
@@ -210,13 +218,52 @@
 TestFileURI = URI::createFile(TestFileName).toString();
   }
 
+  bool collectSymbols(StringRef HeaderCode, StringRef MainCode,
+  const std::vector &ExtraArgs = {}) {
+index::IndexingOptions IndexOpts;
+IndexOpts.SystemSymbolFilter =
+index::IndexingOptions::SystemSymbolFilterKind::All;
+IndexOpts.IndexFunctionLocals = false;
+CollectorOpts.SymOpts = &CollectSymOpts;
+CollectorOpts.OccurrenceOpts = nullptr;
+return runSymbolCollector(HeaderCode, MainCode, CollectorOpts, IndexOpts,
+  ExtraArgs);
+  }
+
+  bool collectOccurrences(StringRef HeaderCode, StringRef MainCode,
+  const std::vector &ExtraArgs = {}) {
+index::IndexingOptions IndexOpts;
+IndexOpts.SystemSymbolFilter =
+index::IndexingOptions::SystemSymbolFilterKind::All;
+IndexOpts.IndexFunctionLocals = true;
+CollectorOpts.SymOpts = nullptr;
+CollectorOpts.OccurrenceOpts = &CollectOccurrenceOpts;
+return runSymbolCollector(HeaderCode, MainCode, CollectorOpts, IndexOpts,
+  ExtraArgs);
+  }
+
+protected:
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem;
+  std::string TestHeaderName;
+  std::string TestHeaderURI;
+  std::string TestFileName;
+  std::string TestFileURI;
+  SymbolSlab Symbols;
+  SymbolCollector::Options CollectorOpts;
+  SymbolCollector::Options::CollectSymbolOptions CollectSymOpts;
+  SymbolCollector::Options::CollectOccurrenceOptions CollectOccurrenceOpts;
+  std::unique_ptr PragmaHandler;
+
+private:
   bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
+  SymbolCollector::Options Opts,
+  index::IndexingOptions IndexOpts,
   const std::vector &ExtraArgs = {}) {
 llvm::IntrusiveRefCntPtr Files(
 new FileManager(FileSystemOptions(), InMemoryFileSystem));
 
 auto Factory = llvm::make_unique(
-CollectorOpts, PragmaHandler.get());
+Opts

[PATCH] D50967: [Preamble] Fix an undefined behavior when checking an empty preamble can be reused.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340403: [Preamble] Fix an undefined behavior when checking 
an empty preamble can be… (authored by hokein, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D50967

Files:
  cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp


Index: cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
===
--- cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
+++ cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
@@ -426,8 +426,8 @@
   // new main file.
   if (PreambleBytes.size() != Bounds.Size ||
   PreambleEndsAtStartOfLine != Bounds.PreambleEndsAtStartOfLine ||
-  memcmp(PreambleBytes.data(), MainFileBuffer->getBufferStart(),
- Bounds.Size) != 0)
+  !std::equal(PreambleBytes.begin(), PreambleBytes.end(),
+  MainFileBuffer->getBuffer().begin()))
 return false;
   // The preamble has not changed. We may be able to re-use the precompiled
   // preamble.


Index: cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
===
--- cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
+++ cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
@@ -426,8 +426,8 @@
   // new main file.
   if (PreambleBytes.size() != Bounds.Size ||
   PreambleEndsAtStartOfLine != Bounds.PreambleEndsAtStartOfLine ||
-  memcmp(PreambleBytes.data(), MainFileBuffer->getBufferStart(),
- Bounds.Size) != 0)
+  !std::equal(PreambleBytes.begin(), PreambleBytes.end(),
+  MainFileBuffer->getBuffer().begin()))
 return false;
   // The preamble has not changed. We may be able to re-use the precompiled
   // preamble.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Looks good mostly, a few nits. We should make sure all related comments are 
updated accordingly




Comment at: clangd/CodeComplete.cpp:198
+static std::vector
+getVirtualNonOverridenMethods(const DeclContext *DC, Sema *S) {
+  const auto *CR = llvm::dyn_cast(DC);

Since we are returning `CodeCompletionResult`, maybe naming it like 
`getNonOverridenCompleteionResults` is clearer?



Comment at: clangd/CodeComplete.cpp:222
+  const std::string Name = Method->getNameAsString();
+  const auto it = Overrides.find(Name);
+  bool IsOverriden = false;

nit: Can't we use `Overrides.find(Method->getName())`  and the other places as 
well?



Comment at: clangd/CodeComplete.cpp:224
+  bool IsOverriden = false;
+  if (it != Overrides.end())
+for (auto *MD : it->second) {

nit: use `{}` around the body of `if` -- the one-line for statement is across 
line, adding `{}` around it will improve the readability. 



Comment at: clangd/CodeComplete.cpp:1238
 : SymbolSlab();
 // Merge Sema and Index results, score them, and pick the winners.
+const auto Overrides = getVirtualNonOverridenMethods(

nit: we need to update the comment accordingly.



Comment at: clangd/CodeComplete.cpp:1281
   // Groups overloads if desired, to form CompletionCandidate::Bundles.
   // The bundles are scored and top results are returned, best to worst.
   std::vector

here as well.



Comment at: clangd/CodeComplete.cpp:1322
   AddToBundles(&SemaResult, CorrespondingIndexResult(SemaResult));
+for (auto &OverrideResult : OverrideResults)
+  AddToBundles(&OverrideResult, CorrespondingIndexResult(OverrideResult),

IIUC, we are treating the override results the same `SemaResult`, it is safe as 
long as Sema doesn't provide these overrides in its code completion results, 
otherwise we will have duplicated completion items?

I think we probably need a proper comment explaining what's going on here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50898



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

[PATCH] D51100: [clang-tidy] Add Abseil prefix to documentation

[Haojian Wu via Phabricator via cfe-commits]

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

LGTM.


https://reviews.llvm.org/D51100



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

[PATCH] D50862: [clang-tidy] Abseil: faster strsplit delimiter check

[Haojian Wu via Phabricator via cfe-commits]

hokein closed this revision.
hokein added a comment.

Committed in https://reviews.llvm.org/rL340411.


https://reviews.llvm.org/D50862



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

[PATCH] D51100: [clang-tidy] Add Abseil prefix to documentation

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340412: [clang-tidy] Add Abseil prefix to documentation 
(authored by hokein, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51100?vs=161940&id=161949#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51100

Files:
  clang-tools-extra/trunk/docs/clang-tidy/index.rst


Index: clang-tools-extra/trunk/docs/clang-tidy/index.rst
===
--- clang-tools-extra/trunk/docs/clang-tidy/index.rst
+++ clang-tools-extra/trunk/docs/clang-tidy/index.rst
@@ -55,6 +55,7 @@
 == 
=
 Name prefixDescription
 == 
=
+``abseil-``Checks related to Abseil library.
 ``android-``   Checks related to Android.
 ``boost-`` Checks related to Boost library.
 ``bugprone-``  Checks that target bugprone code constructs.


Index: clang-tools-extra/trunk/docs/clang-tidy/index.rst
===
--- clang-tools-extra/trunk/docs/clang-tidy/index.rst
+++ clang-tools-extra/trunk/docs/clang-tidy/index.rst
@@ -55,6 +55,7 @@
 == =
 Name prefixDescription
 == =
+``abseil-``Checks related to Abseil library.
 ``android-``   Checks related to Android.
 ``boost-`` Checks related to Boost library.
 ``bugprone-``  Checks that target bugprone code constructs.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 161962.
hokein marked 6 inline comments as done.
hokein added a comment.

Address review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385

Files:
  clangd/index/FileIndex.cpp
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -74,6 +74,14 @@
 MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
   return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
 }
+MATCHER(OccurrenceRange, "") {
+  const clang::clangd::SymbolOccurrence &Pos = testing::get<0>(arg);
+  const clang::clangd::Range &Range = testing::get<1>(arg);
+  return std::tie(Pos.Location.Start.Line, Pos.Location.Start.Column,
+  Pos.Location.End.Line, Pos.Location.End.Column) ==
+ std::tie(Range.start.line, Range.start.character, Range.end.line,
+  Range.end.character);
+}
 
 namespace clang {
 namespace clangd {
@@ -95,7 +103,7 @@
 assert(AST.hasValue());
 return SymbolCollector::shouldCollectSymbol(
 Qualified ? findDecl(*AST, Name) : findAnyDecl(*AST, Name),
-AST->getASTContext(), SymbolCollector::Options());
+AST->getASTContext());
   }
 
 protected:
@@ -162,8 +170,10 @@
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
   SymbolIndexActionFactory(SymbolCollector::Options COpts,
-   CommentHandler *PragmaHandler)
-  : COpts(std::move(COpts)), PragmaHandler(PragmaHandler) {}
+   CommentHandler *PragmaHandler,
+   index::IndexingOptions IndexOpts)
+  : COpts(std::move(COpts)), PragmaHandler(PragmaHandler),
+IndexOpts(IndexOpts) {}
 
   clang::FrontendAction *create() override {
 class WrappedIndexAction : public WrapperFrontendAction {
@@ -186,18 +196,16 @@
   index::IndexingOptions IndexOpts;
   CommentHandler *PragmaHandler;
 };
-index::IndexingOptions IndexOpts;
-IndexOpts.SystemSymbolFilter =
-index::IndexingOptions::SystemSymbolFilterKind::All;
-IndexOpts.IndexFunctionLocals = false;
 Collector = std::make_shared(COpts);
 return new WrappedIndexAction(Collector, std::move(IndexOpts),
   PragmaHandler);
   }
 
   std::shared_ptr Collector;
   SymbolCollector::Options COpts;
   CommentHandler *PragmaHandler;
+
+  index::IndexingOptions IndexOpts;
 };
 
 class SymbolCollectorTest : public ::testing::Test {
@@ -210,13 +218,52 @@
 TestFileURI = URI::createFile(TestFileName).toString();
   }
 
+  bool collectSymbols(StringRef HeaderCode, StringRef MainCode,
+  const std::vector &ExtraArgs = {}) {
+index::IndexingOptions IndexOpts;
+IndexOpts.SystemSymbolFilter =
+index::IndexingOptions::SystemSymbolFilterKind::All;
+IndexOpts.IndexFunctionLocals = false;
+CollectorOpts.SymOpts = &CollectSymOpts;
+CollectorOpts.OccurrenceOpts = nullptr;
+return runSymbolCollector(HeaderCode, MainCode, CollectorOpts, IndexOpts,
+  ExtraArgs);
+  }
+
+  bool collectOccurrences(StringRef HeaderCode, StringRef MainCode,
+  const std::vector &ExtraArgs = {}) {
+index::IndexingOptions IndexOpts;
+IndexOpts.SystemSymbolFilter =
+index::IndexingOptions::SystemSymbolFilterKind::All;
+IndexOpts.IndexFunctionLocals = true;
+CollectorOpts.SymOpts = nullptr;
+CollectorOpts.OccurrenceOpts = &CollectOccurrenceOpts;
+return runSymbolCollector(HeaderCode, MainCode, CollectorOpts, IndexOpts,
+  ExtraArgs);
+  }
+
+protected:
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem;
+  std::string TestHeaderName;
+  std::string TestHeaderURI;
+  std::string TestFileName;
+  std::string TestFileURI;
+  SymbolSlab Symbols;
+  SymbolCollector::Options CollectorOpts;
+  SymbolCollector::Options::CollectSymbolOptions CollectSymOpts;
+  SymbolCollector::Options::CollectOccurrenceOptions CollectOccurrenceOpts;
+  std::unique_ptr PragmaHandler;
+
+private:
   bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
+  SymbolCollector::Options Opts,
+  index::IndexingOptions IndexOpts,
   const std::vector &ExtraArgs = {}) {
 llvm::IntrusiveRefCntPtr Files(
 new FileManager(FileSystemOptions(), InMemoryFileSystem));
 
 auto Factory = llvm::make_unique(
-CollectorOpts, PragmaHandler.get());
+Opts, PragmaHandler.get(), std::move(IndexOpts));
 
 std::vector Args = {
 "symbol_collector", "-fsyntax-only", "-xc++",
@@ -239,16 +286,6 @@
 Symbols 

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 161972.
hokein marked an inline comment as done.
hokein added a comment.

Add one more comment.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385

Files:
  clangd/index/FileIndex.cpp
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -74,6 +74,14 @@
 MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
   return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
 }
+MATCHER(OccurrenceRange, "") {
+  const clang::clangd::SymbolOccurrence &Pos = testing::get<0>(arg);
+  const clang::clangd::Range &Range = testing::get<1>(arg);
+  return std::tie(Pos.Location.Start.Line, Pos.Location.Start.Column,
+  Pos.Location.End.Line, Pos.Location.End.Column) ==
+ std::tie(Range.start.line, Range.start.character, Range.end.line,
+  Range.end.character);
+}
 
 namespace clang {
 namespace clangd {
@@ -95,7 +103,7 @@
 assert(AST.hasValue());
 return SymbolCollector::shouldCollectSymbol(
 Qualified ? findDecl(*AST, Name) : findAnyDecl(*AST, Name),
-AST->getASTContext(), SymbolCollector::Options());
+AST->getASTContext());
   }
 
 protected:
@@ -162,8 +170,10 @@
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
   SymbolIndexActionFactory(SymbolCollector::Options COpts,
-   CommentHandler *PragmaHandler)
-  : COpts(std::move(COpts)), PragmaHandler(PragmaHandler) {}
+   CommentHandler *PragmaHandler,
+   index::IndexingOptions IndexOpts)
+  : COpts(std::move(COpts)), PragmaHandler(PragmaHandler),
+IndexOpts(IndexOpts) {}
 
   clang::FrontendAction *create() override {
 class WrappedIndexAction : public WrapperFrontendAction {
@@ -186,18 +196,16 @@
   index::IndexingOptions IndexOpts;
   CommentHandler *PragmaHandler;
 };
-index::IndexingOptions IndexOpts;
-IndexOpts.SystemSymbolFilter =
-index::IndexingOptions::SystemSymbolFilterKind::All;
-IndexOpts.IndexFunctionLocals = false;
 Collector = std::make_shared(COpts);
 return new WrappedIndexAction(Collector, std::move(IndexOpts),
   PragmaHandler);
   }
 
   std::shared_ptr Collector;
   SymbolCollector::Options COpts;
   CommentHandler *PragmaHandler;
+
+  index::IndexingOptions IndexOpts;
 };
 
 class SymbolCollectorTest : public ::testing::Test {
@@ -210,13 +218,52 @@
 TestFileURI = URI::createFile(TestFileName).toString();
   }
 
+  bool collectSymbols(StringRef HeaderCode, StringRef MainCode,
+  const std::vector &ExtraArgs = {}) {
+index::IndexingOptions IndexOpts;
+IndexOpts.SystemSymbolFilter =
+index::IndexingOptions::SystemSymbolFilterKind::All;
+IndexOpts.IndexFunctionLocals = false;
+CollectorOpts.SymOpts = &CollectSymOpts;
+CollectorOpts.OccurrenceOpts = nullptr;
+return runSymbolCollector(HeaderCode, MainCode, CollectorOpts, IndexOpts,
+  ExtraArgs);
+  }
+
+  bool collectOccurrences(StringRef HeaderCode, StringRef MainCode,
+  const std::vector &ExtraArgs = {}) {
+index::IndexingOptions IndexOpts;
+IndexOpts.SystemSymbolFilter =
+index::IndexingOptions::SystemSymbolFilterKind::All;
+IndexOpts.IndexFunctionLocals = true;
+CollectorOpts.SymOpts = nullptr;
+CollectorOpts.OccurrenceOpts = &CollectOccurrenceOpts;
+return runSymbolCollector(HeaderCode, MainCode, CollectorOpts, IndexOpts,
+  ExtraArgs);
+  }
+
+protected:
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem;
+  std::string TestHeaderName;
+  std::string TestHeaderURI;
+  std::string TestFileName;
+  std::string TestFileURI;
+  SymbolSlab Symbols;
+  SymbolCollector::Options CollectorOpts;
+  SymbolCollector::Options::CollectSymbolOptions CollectSymOpts;
+  SymbolCollector::Options::CollectOccurrenceOptions CollectOccurrenceOpts;
+  std::unique_ptr PragmaHandler;
+
+private:
   bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
+  SymbolCollector::Options Opts,
+  index::IndexingOptions IndexOpts,
   const std::vector &ExtraArgs = {}) {
 llvm::IntrusiveRefCntPtr Files(
 new FileManager(FileSystemOptions(), InMemoryFileSystem));
 
 auto Factory = llvm::make_unique(
-CollectorOpts, PragmaHandler.get());
+Opts, PragmaHandler.get(), std::move(IndexOpts));
 
 std::vector Args = {
 "symbol_collector", "-fsyntax-only", "-xc++",
@@ -239,16 +286,6 @@
 Symbols = F

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/index/Index.h:46
+
+inline bool operator==(const SymbolLocation::Position &L,
+   const SymbolLocation::Position &R) {

ilya-biryukov wrote:
> NIT: having friend decls inside the classes themselves might prove to be more 
> readable.
> Not opposed to the current one too, feel free to ignore.
These operator implementations seem not as much interesting as members in the 
structure, putting them to the structure probably adds some noise to readers.





Comment at: clangd/index/Index.h:350
+
+llvm::DenseMap> SymbolOccurrences;
   };

ilya-biryukov wrote:
> Any store occurences in a file-centric manner?
> E.g. 
> ```
> /// Occurences inside a single file.
> class FileOccurences {
>   StringRef File;
>   vector> Locations;
> };
> 
> // 
> DenseMap>  SymbolOccurences;
> ```
> 
> As discussed previously, this representation is better suited for both 
> merging and serialization.
The file-centric manner doesn't seem to suite our current model:  whenever we 
update the index for the main AST, we just replace the symbol slab with the new 
one; and for index merging, we only use the index `findOccurrences` interfaces.

It would save some memory usage of `StringRef` File, but AFAIK, the memory 
usage of current model is relatively small (comparing with the SymbolSlab for 
code completion) since we only store occurrences in main file (~50KB for 
`CodeComplete.cpp`).

I'd leave it as it is now, and we could revisit it later.



Comment at: clangd/index/SymbolCollector.h:59
+  // If none, all symbol occurrences will be collected.
+  llvm::Optional> IDs = llvm::None;
+};

ilya-biryukov wrote:
> Could you elaborate on what this option will be used for? How do we know in 
> advance which symbols we're interested in?
This is used for finding references in the AST as a part of the xref 
implementation, basically the workflow would be:

1. find SymbolIDs of the symbol under the cursor, using 
`DeclarationAndMacrosFinder`
2. run symbol collector to find all occurrences in the main AST with all 
SymbolIDs in #1
3. query the index, to get more occurrences
4. merge them  


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



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

[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.

[Haojian Wu via Phabricator via cfe-commits]

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

Thanks, LGTM.




Comment at: clangd/CodeComplete.cpp:210
+const std::string Name = Method->getNameAsString();
+Overrides[Name].push_back(Method);
+  }

nit: here as well, use `Overrides[Method->getName()].push_back...`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50898



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

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Haojian Wu via Phabricator via cfe-commits]

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

Looks good.




Comment at: test/clang-tidy/abseil-no-namespace.cpp:10
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:11: warning: namespace 'absl' is reserved
+// for implementation of the Abseil library and should not be opened in user

Does the test get passed on the first command `%check_clang_tidy %s 
abseil-no-namespace %t -- -- -I %S/Inputs`? The first command will suppress all 
warning in headers, I think it will fail?


https://reviews.llvm.org/D50580



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

[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.

still LG.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50898



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

[PATCH] D46684: [Frontend] Don't skip function body when the return type is dependent on the template parameter.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.

Otherwise clang will provide an unexpected diagnostic error.


Repository:
  rC Clang

https://reviews.llvm.org/D46684

Files:
  lib/Sema/SemaDecl.cpp
  test/Index/skipped-auto-fn-templates.cpp


Index: test/Index/skipped-auto-fn-templates.cpp
===
--- /dev/null
+++ test/Index/skipped-auto-fn-templates.cpp
@@ -0,0 +1,10 @@
+// RUN: env CINDEXTEST_SKIP_FUNCTION_BODIES=1 c-index-test -test-load-source 
all %s 2>&1 \
+// RUN: | FileCheck %s
+
+template 
+auto foo(T a) {
+  return a;
+}
+
+int b = foo(0);
+// CHECK-NOT: error: function 'foo' with deduced return type cannot be 
used before it is defined
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12637,8 +12637,12 @@
   // rest of the file.
   // We cannot skip the body of a function with an undeduced return type,
   // because any callers of that function need to know the type.
+  // We cannot skip the body of a function template with an 'auto' return type
+  // which is dependent on a template parameter, we need to see the function
+  // body before using it.
   if (const FunctionDecl *FD = D->getAsFunction())
-if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType())
+if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType() ||
+FD->getReturnType()->isInstantiationDependentType())
   return false;
   return Consumer.shouldSkipFunctionBody(D);
 }


Index: test/Index/skipped-auto-fn-templates.cpp
===
--- /dev/null
+++ test/Index/skipped-auto-fn-templates.cpp
@@ -0,0 +1,10 @@
+// RUN: env CINDEXTEST_SKIP_FUNCTION_BODIES=1 c-index-test -test-load-source all %s 2>&1 \
+// RUN: | FileCheck %s
+
+template 
+auto foo(T a) {
+  return a;
+}
+
+int b = foo(0);
+// CHECK-NOT: error: function 'foo' with deduced return type cannot be used before it is defined
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12637,8 +12637,12 @@
   // rest of the file.
   // We cannot skip the body of a function with an undeduced return type,
   // because any callers of that function need to know the type.
+  // We cannot skip the body of a function template with an 'auto' return type
+  // which is dependent on a template parameter, we need to see the function
+  // body before using it.
   if (const FunctionDecl *FD = D->getAsFunction())
-if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType())
+if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType() ||
+FD->getReturnType()->isInstantiationDependentType())
   return false;
   return Consumer.shouldSkipFunctionBody(D);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38402: [clang-refactor] Apply source replacements

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: include/clang/Frontend/CommandLineSourceLoc.h:57
+  std::string FileName;
+  std::pair Begin;
+  std::pair End;

Add a comment documenting what the first element and second element of the pair 
represent for (? If I understand correctly).



Comment at: include/clang/Frontend/CommandLineSourceLoc.h:60
+
+  /// Returns a parsed source range from a string or None if the string is
+  /// invalid.

Would be clearer to document the valid format of the `Str`. 



Comment at: test/Refactor/tool-apply-replacements.cpp:3
+// RUN: cp %s %t.cp.cpp
+// RUN: clang-refactor local-rename -selection=%t.cp.cpp:6:7 -new-name=test 
%t.cp.cpp --
+// RUN: grep -v CHECK %t.cp.cpp | FileCheck %t.cp.cpp

Maybe add one more case for testing `-selection=%t.cp.cpp:6:7-6:15`.



Comment at: tools/clang-refactor/ClangRefactor.cpp:309
 public:
   void handleError(llvm::Error Err) {
 llvm::errs() << llvm::toString(std::move(Err)) << "\n";

Add `override`.



Comment at: tools/clang-refactor/ClangRefactor.cpp:313
 
-  // FIXME: Consume atomic changes and apply them to files.
+  void handle(AtomicChanges Changes) {
+SourceChanges.insert(SourceChanges.begin(), Changes.begin(), 
Changes.end());

The same, `override`.



Comment at: tools/clang-refactor/ClangRefactor.cpp:412
+  if (!BufferErr) {
+llvm::errs() << "error: failed to open" << File << " for rewriting\n";
+return true;

nit: missing a blank after `open`.


Repository:
  rL LLVM

https://reviews.llvm.org/D38402



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

[PATCH] D38723: [clang-rename] Don't add prefix qualifiers to the declaration and definition of the renamed symbol.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
Herald added a subscriber: klimek.

https://reviews.llvm.org/D38723

Files:
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  unittests/Rename/RenameClassTest.cpp

Index: unittests/Rename/RenameClassTest.cpp
===
--- unittests/Rename/RenameClassTest.cpp
+++ unittests/Rename/RenameClassTest.cpp
@@ -469,8 +469,6 @@
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being added to the class definition and
-// constructor.
 TEST_F(ClangRenameTest, RenameClassWithNamespaceWithInlineMembers) {
   std::string Before = R"(
   namespace ns {
@@ -488,9 +486,9 @@
 )";
   std::string Expected = R"(
   namespace ns {
-  class ns::New {
+  class New {
public:
-ns::New() {}
+New() {}
 ~New() {}
 
 New* next() { return next_; }
@@ -504,8 +502,6 @@
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being added to the class definition and
-// constructor.
 TEST_F(ClangRenameTest, RenameClassWithNamespaceWithOutOfInlineMembers) {
   std::string Before = R"(
   namespace ns {
@@ -527,32 +523,32 @@
 )";
   std::string Expected = R"(
   namespace ns {
-  class ns::New {
+  class New {
public:
-ns::New();
+New();
 ~New();
 
 New* next();
 
private:
 New* next_;
   };
 
-  New::ns::New() {}
+  New::New() {}
   New::~New() {}
   New* New::next() { return next_; }
   }  // namespace ns
 )";
   std::string After = runClangRenameOnCode(Before, "ns::Old", "ns::New");
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being added to the definition.
 TEST_F(ClangRenameTest, RenameClassInInheritedConstructor) {
   // `using Base::Base;` will generate an implicit constructor containing usage
   // of `::ns::Old` which should not be matched.
   std::string Before = R"(
   namespace ns {
+  class Old;
   class Old {
 int x;
   };
@@ -574,7 +570,8 @@
   })";
   std::string Expected = R"(
   namespace ns {
-  class ns::New {
+  class New;
+  class New {
 int x;
   };
   class Base {
@@ -615,7 +612,7 @@
   )";
   std::string Expected = R"(
   namespace ns {
-  class ::new_ns::New {
+  class New {
   };
   } // namespace ns
   struct S {
@@ -632,7 +629,6 @@
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being adding to the definition.
 TEST_F(ClangRenameTest, ReferencesInLambdaFunctionParameters) {
   std::string Before = R"(
   template 
@@ -669,7 +665,7 @@
   };
 
   namespace ns {
-  class ::new_ns::New {};
+  class New {};
   void f() {
 function func;
   }
Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -160,13 +160,13 @@
 const Decl *Context;
 // The nested name being replaced (can be nullptr).
 const NestedNameSpecifier *Specifier;
+// Determine whether the prefix qualifiers of the NewName are known to be
+// ignored.
+// For example, if it is true and NewName is "a::b::foo", then the symbol
+// will be renamed to "foo".
+bool IgnorePrefixQualifers;
   };
 
-  // FIXME: Currently, prefix qualifiers will be added to the renamed symbol
-  // definition (e.g. "class Foo {};" => "class b::Bar {};" when renaming
-  // "a::Foo" to "b::Bar").
-  // For renaming declarations/definitions, prefix qualifiers should be filtered
-  // out.
   bool VisitNamedDecl(const NamedDecl *Decl) {
 // UsingDecl has been handled in other place.
 if (llvm::isa(Decl))
@@ -180,8 +180,12 @@
   return true;
 
 if (isInUSRSet(Decl)) {
-  RenameInfo Info = {Decl->getLocation(), Decl->getLocation(), nullptr,
- nullptr, nullptr};
+  RenameInfo Info = {Decl->getLocation(),
+ Decl->getLocation(),
+ /*FromDecl=*/nullptr,
+ /*Context=*/nullptr,
+ /*Specifier=*/nullptr,
+ /*IgnorePrefixQualifers=*/true};
   RenameInfos.push_back(Info);
 }
 return true;
@@ -191,8 +195,11 @@
 const NamedDecl *Decl = Expr->getFoundDecl();
 if (isInUSRSet(Decl)) {
   RenameInfo Info = {Expr->getSourceRange().getBegin(),
- Expr->getSourceRange().getEnd(), Decl,
- getClosestAncestorDecl(*Expr), Expr->getQualifier()};
+ Expr->getSourceRange().getEnd(),
+ Decl,
+ getClosestAncestorDecl(*Expr),
+ Expr->getQualifier(),
+ /*IgnorePrefixQualifers=*/false};
   RenameIn

[PATCH] D38402: [clang-refactor] Apply source replacements

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.

LGTM, let's check in it.


Repository:
  rL LLVM

https://reviews.llvm.org/D38402



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

[PATCH] D38772: [refactor] allow the use of refactoring diagnostics

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

The code looks most good to me, a few nits.




Comment at: lib/Basic/DiagnosticIDs.cpp:46
   unsigned WarnShowInSystemHeader : 1;
-  unsigned Category : 5;
+  unsigned Category : 6;
 

just curious: is this change needed?



Comment at: tools/clang-refactor/ToolRefactoringResultConsumer.h:19
+
+/// A subclass of \c RefactoringResultConsumer that stores the reference to the
+/// TU-specific diagnostics engine.

I'd name it "interface", because it has unimplemented virtual function 
(`handleError`), clients can't create an instance of it. 

or alternatively,  does it make more sense to just add these methods and 
`DiagnosticsEngine` variable to the `tooling::RefactoringResultConsumer` 
interface? I see you have replaced "RefactoringResultConsumer" with this new 
interface in many places. 


Repository:
  rL LLVM

https://reviews.llvm.org/D38772



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

[PATCH] D38723: [clang-rename] Don't add prefix qualifiers to the declaration and definition of the renamed symbol.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 118573.
hokein marked 3 inline comments as done.
hokein added a comment.

Address review comments.


https://reviews.llvm.org/D38723

Files:
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  unittests/Rename/RenameClassTest.cpp

Index: unittests/Rename/RenameClassTest.cpp
===
--- unittests/Rename/RenameClassTest.cpp
+++ unittests/Rename/RenameClassTest.cpp
@@ -469,8 +469,6 @@
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being added to the class definition and
-// constructor.
 TEST_F(ClangRenameTest, RenameClassWithNamespaceWithInlineMembers) {
   std::string Before = R"(
   namespace ns {
@@ -488,9 +486,9 @@
 )";
   std::string Expected = R"(
   namespace ns {
-  class ns::New {
+  class New {
public:
-ns::New() {}
+New() {}
 ~New() {}
 
 New* next() { return next_; }
@@ -504,8 +502,6 @@
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being added to the class definition and
-// constructor.
 TEST_F(ClangRenameTest, RenameClassWithNamespaceWithOutOfInlineMembers) {
   std::string Before = R"(
   namespace ns {
@@ -527,32 +523,32 @@
 )";
   std::string Expected = R"(
   namespace ns {
-  class ns::New {
+  class New {
public:
-ns::New();
+New();
 ~New();
 
 New* next();
 
private:
 New* next_;
   };
 
-  New::ns::New() {}
+  New::New() {}
   New::~New() {}
   New* New::next() { return next_; }
   }  // namespace ns
 )";
   std::string After = runClangRenameOnCode(Before, "ns::Old", "ns::New");
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being added to the definition.
 TEST_F(ClangRenameTest, RenameClassInInheritedConstructor) {
   // `using Base::Base;` will generate an implicit constructor containing usage
   // of `::ns::Old` which should not be matched.
   std::string Before = R"(
   namespace ns {
+  class Old;
   class Old {
 int x;
   };
@@ -574,7 +570,8 @@
   })";
   std::string Expected = R"(
   namespace ns {
-  class ns::New {
+  class New;
+  class New {
 int x;
   };
   class Base {
@@ -615,7 +612,7 @@
   )";
   std::string Expected = R"(
   namespace ns {
-  class ::new_ns::New {
+  class New {
   };
   } // namespace ns
   struct S {
@@ -632,7 +629,6 @@
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being adding to the definition.
 TEST_F(ClangRenameTest, ReferencesInLambdaFunctionParameters) {
   std::string Before = R"(
   template 
@@ -669,7 +665,7 @@
   };
 
   namespace ns {
-  class ::new_ns::New {};
+  class New {};
   void f() {
 function func;
   }
Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -160,13 +160,15 @@
 const Decl *Context;
 // The nested name being replaced (can be nullptr).
 const NestedNameSpecifier *Specifier;
+// Determine whether the prefix qualifiers of the NewName should be ignored.
+// Normally, we set it to true for the symbol declaration and definition to
+// avoid adding prefix qualifiers.
+// For example, if it is true and NewName is
+// "a::b::foo", then the symbol occurrence which the RenameInfo points to
+// will be renamed to "foo".
+bool IgnorePrefixQualifers;
   };
 
-  // FIXME: Currently, prefix qualifiers will be added to the renamed symbol
-  // definition (e.g. "class Foo {};" => "class b::Bar {};" when renaming
-  // "a::Foo" to "b::Bar").
-  // For renaming declarations/definitions, prefix qualifiers should be filtered
-  // out.
   bool VisitNamedDecl(const NamedDecl *Decl) {
 // UsingDecl has been handled in other place.
 if (llvm::isa(Decl))
@@ -180,8 +182,12 @@
   return true;
 
 if (isInUSRSet(Decl)) {
-  RenameInfo Info = {Decl->getLocation(), Decl->getLocation(), nullptr,
- nullptr, nullptr};
+  RenameInfo Info = {Decl->getLocation(),
+ Decl->getLocation(),
+ /*FromDecl=*/nullptr,
+ /*Context=*/nullptr,
+ /*Specifier=*/nullptr,
+ /*IgnorePrefixQualifers=*/true};
   RenameInfos.push_back(Info);
 }
 return true;
@@ -191,8 +197,11 @@
 const NamedDecl *Decl = Expr->getFoundDecl();
 if (isInUSRSet(Decl)) {
   RenameInfo Info = {Expr->getSourceRange().getBegin(),
- Expr->getSourceRange().getEnd(), Decl,
- getClosestAncestorDecl(*Expr), Expr->getQualifier()};
+ Expr->getSourceR

[PATCH] D38723: [clang-rename] Don't add prefix qualifiers to the declaration and definition of the renamed symbol.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL315452: [clang-rename] Don't add prefix qualifiers to the 
declaration and definition of… (authored by hokein).

Changed prior to commit:
  https://reviews.llvm.org/D38723?vs=118573&id=118587#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38723

Files:
  cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  cfe/trunk/unittests/Rename/RenameClassTest.cpp

Index: cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -160,13 +160,14 @@
 const Decl *Context;
 // The nested name being replaced (can be nullptr).
 const NestedNameSpecifier *Specifier;
+// Determine whether the prefix qualifiers of the NewName should be ignored.
+// Normally, we set it to true for the symbol declaration and definition to
+// avoid adding prefix qualifiers.
+// For example, if it is true and NewName is "a::b::foo", then the symbol
+// occurrence which the RenameInfo points to will be renamed to "foo".
+bool IgnorePrefixQualifers;
   };
 
-  // FIXME: Currently, prefix qualifiers will be added to the renamed symbol
-  // definition (e.g. "class Foo {};" => "class b::Bar {};" when renaming
-  // "a::Foo" to "b::Bar").
-  // For renaming declarations/definitions, prefix qualifiers should be filtered
-  // out.
   bool VisitNamedDecl(const NamedDecl *Decl) {
 // UsingDecl has been handled in other place.
 if (llvm::isa(Decl))
@@ -180,8 +181,12 @@
   return true;
 
 if (isInUSRSet(Decl)) {
-  RenameInfo Info = {Decl->getLocation(), Decl->getLocation(), nullptr,
- nullptr, nullptr};
+  RenameInfo Info = {Decl->getLocation(),
+ Decl->getLocation(),
+ /*FromDecl=*/nullptr,
+ /*Context=*/nullptr,
+ /*Specifier=*/nullptr,
+ /*IgnorePrefixQualifers=*/true};
   RenameInfos.push_back(Info);
 }
 return true;
@@ -191,8 +196,11 @@
 const NamedDecl *Decl = Expr->getFoundDecl();
 if (isInUSRSet(Decl)) {
   RenameInfo Info = {Expr->getSourceRange().getBegin(),
- Expr->getSourceRange().getEnd(), Decl,
- getClosestAncestorDecl(*Expr), Expr->getQualifier()};
+ Expr->getSourceRange().getEnd(),
+ Decl,
+ getClosestAncestorDecl(*Expr),
+ Expr->getQualifier(),
+ /*IgnorePrefixQualifers=*/false};
   RenameInfos.push_back(Info);
 }
 
@@ -220,8 +228,10 @@
   if (isInUSRSet(TargetDecl)) {
 RenameInfo Info = {NestedLoc.getBeginLoc(),
EndLocationForType(NestedLoc.getTypeLoc()),
-   TargetDecl, getClosestAncestorDecl(NestedLoc),
-   NestedLoc.getNestedNameSpecifier()->getPrefix()};
+   TargetDecl,
+   getClosestAncestorDecl(NestedLoc),
+   NestedLoc.getNestedNameSpecifier()->getPrefix(),
+   /*IgnorePrefixQualifers=*/false};
 RenameInfos.push_back(Info);
   }
 }
@@ -265,9 +275,12 @@
 if (!ParentTypeLoc.isNull() &&
 isInUSRSet(getSupportedDeclFromTypeLoc(ParentTypeLoc)))
   return true;
-RenameInfo Info = {StartLocationForType(Loc), EndLocationForType(Loc),
-   TargetDecl, getClosestAncestorDecl(Loc),
-   GetNestedNameForType(Loc)};
+RenameInfo Info = {StartLocationForType(Loc),
+   EndLocationForType(Loc),
+   TargetDecl,
+   getClosestAncestorDecl(Loc),
+   GetNestedNameForType(Loc),
+   /*IgnorePrefixQualifers=*/false};
 RenameInfos.push_back(Info);
 return true;
   }
@@ -293,11 +306,13 @@
 llvm::isa(ParentTypeLoc.getType()))
   TargetLoc = ParentTypeLoc;
 RenameInfo Info = {
-StartLocationForType(TargetLoc), EndLocationForType(TargetLoc),
+StartLocationForType(TargetLoc),
+EndLocationForType(TargetLoc),
 TemplateSpecType->getTemplateName().getAsTemplateDecl(),
 getClosestAncestorDecl(
 ast_type_traits::DynTypedNode::create(TargetLoc)),
-GetNestedNameForType(TargetLoc)};
+GetNestedNameForType(TargetLoc),
+/*IgnorePrefixQualifers=*/false};
 RenameInfos.push_back(Info);
   }
 }
@@ -423,18 +438,26 @@
 
   for (const auto &RenameInfo : Finder.getRenameInfos()) {
 std::string ReplacedName = 

[PATCH] D37856: [refactor] add support for refactoring options

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Sorry for the delay. I saw you have reverted this commit somehow. A post commit.




Comment at: cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp:113
 Rules.push_back(createRefactoringActionRule(
-SymbolSelectionRequirement()));
+SymbolSelectionRequirement(), OptionRequirement()));
 return Rules;

Thought it a bit more: it requires all of the requirements are satisfied, I 
think we need to support "one-of" option. For example,  we have two option "-a" 
and "-b",  only one of them is allowed to be present at the same time.


Repository:
  rL LLVM

https://reviews.llvm.org/D37856



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

[PATCH] D37856: [refactor] add support for refactoring options

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp:113
 Rules.push_back(createRefactoringActionRule(
-SymbolSelectionRequirement()));
+SymbolSelectionRequirement(), OptionRequirement()));
 return Rules;

arphaman wrote:
> hokein wrote:
> > Thought it a bit more: it requires all of the requirements are satisfied, I 
> > think we need to support "one-of" option. For example,  we have two option 
> > "-a" and "-b",  only one of them is allowed to be present at the same time.
> That should be straightforward enough to implement, either with a custom 
> class or with a built-in requirement class. I'll probably add a builtin one 
> when the need comes up.
Thanks. I'm asking it because I plan to add the `-qualified-name` option to the 
clang-refactor rename subtool. And obviously "-qualified-name" can not be 
present with "-selection".


Repository:
  rL LLVM

https://reviews.llvm.org/D37856



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