[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-19 Thread Eric Liu via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE342529: [clangd] Store preamble macros in dynamic index. 
(authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52078?vs=165959=166078#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078

Files:
  clangd/index/FileIndex.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -223,12 +223,13 @@
 void TestAfterDotCompletion(clangd::CodeCompleteOptions Opts) {
   auto Results = completions(
   R"cpp(
-  #define MACRO X
-
   int global_var;
 
   int global_func();
 
+  // Make sure this is not in preamble.
+  #define MACRO X
+
   struct GlobalClass {};
 
   struct ClassWithMembers {
@@ -276,11 +277,12 @@
 void TestGlobalScopeCompletion(clangd::CodeCompleteOptions Opts) {
   auto Results = completions(
   R"cpp(
-  #define MACRO X
-
   int global_var;
   int global_func();
 
+  // Make sure this is not in preamble.
+  #define MACRO X
+
   struct GlobalClass {};
 
   struct ClassWithMembers {
@@ -430,10 +432,11 @@
 TEST(CompletionTest, Kinds) {
   auto Results = completions(
   R"cpp(
-  #define MACRO X
   int variable;
   struct Struct {};
   int function();
+  // make sure MACRO is not included in preamble.
+  #define MACRO 10
   int X = ^
   )cpp",
   {func("indexFunction"), var("indexVariable"), cls("indexClass")});
@@ -1921,6 +1924,21 @@
   UnorderedElementsAre(Named("Clangd_Macro_Test")));
 }
 
+TEST(CompletionTest, NoMacroFromPreambleIfIndexIsSet) {
+  auto Results = completions(
+  R"cpp(#define CLANGD_PREAMBLE x
+
+  int x = 0;
+  #define CLANGD_MAIN x
+  void f() { CLANGD_^ }
+  )cpp",
+  {func("CLANGD_INDEX")});
+  // Index is overriden in code completion options, so the preamble symbol is
+  // not seen.
+  EXPECT_THAT(Results.Completions, UnorderedElementsAre(Named("CLANGD_MAIN"),
+Named("CLANGD_INDEX")));
+}
+
 TEST(CompletionTest, DeprecatedResults) {
   std::string Body = R"cpp(
 void TestClangd();
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -15,6 +15,7 @@
 #include "index/FileIndex.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Index/IndexSymbol.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "gtest/gtest.h"
@@ -330,6 +331,21 @@
  FileURI("unittest:///test2.cc"))}));
 }
 
+TEST(FileIndexTest, CollectMacros) {
+  FileIndex M;
+  update(M, "f", "#define CLANGD 1");
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  bool SeenSymbol = false;
+  M.index().fuzzyFind(Req, [&](const Symbol ) {
+EXPECT_EQ(Sym.Name, "CLANGD");
+EXPECT_EQ(Sym.SymInfo.Kind, index::SymbolKind::Macro);
+SeenSymbol = true;
+  });
+  EXPECT_TRUE(SeenSymbol);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/index/FileIndex.cpp
===
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -14,6 +14,7 @@
 #include "index/Index.h"
 #include "index/Merge.h"
 #include "clang/Index/IndexingAction.h"
+#include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include 
 
@@ -41,10 +42,13 @@
   IndexOpts.SystemSymbolFilter =
   index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly;
   IndexOpts.IndexFunctionLocals = false;
-
-  if (IsIndexMainAST)
+  if (IsIndexMainAST) {
 // We only collect refs when indexing main AST.
 CollectorOpts.RefFilter = RefKind::All;
+  }else {
+IndexOpts.IndexMacrosInPreprocessor = true;
+CollectorOpts.CollectMacro = true;
+  }
 
   SymbolCollector Collector(std::move(CollectorOpts));
   Collector.setPreprocessor(PP);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-18 Thread Mailing List "cfe-commits" via Phabricator via cfe-commits 
cfe-commits added a comment.



- F7238787: msg-7222-125.txt 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078



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

Re: [PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-18 Thread Sam McCall via cfe-commits 
On Tue, Sep 18, 2018, 16:28 Eric Liu via Phabricator <
revi...@reviews.llvm.org> wrote:

> ioeric added a comment.
>
> In https://reviews.llvm.org/D52078#1238301, @sammccall wrote:
>
> > Something not listed in cons: because macros aren't namespaced and we
> don't have lots of signals, they can be really spammy.
> >  Potentially, offering macros that aren't in the TU could be a loss even
> if it's a win for other types of signals.
>
>
> Aren't they already spammy from Sema? Sema can provide thousands of macros
> in the TU.
>
Indeed, but Sema will at least implicitly restrict to the transitive
headers!

Agree with everything you say, looking forward to this change!

We penalize quality of macro symbols in the global index. Maybe we can do
> the same thing for dynamic index?
>
> > We could always e.g. postfilter index macro results using the include
> structure of the preamble, so no concern for this patch, just something to
> think about for the followup.
>
> Sounds good.
>
> > We also need to make sure that we're not indexing/serving header guards
> as code completions (e.g. if SemaCodeComplete is currently taking care of
> this)
>
> These symbols are already filtered out in `SymbolCollector`.
>
>
> Repository:
>   rCTE Clang Tools Extra
>
> https://reviews.llvm.org/D52078
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-18 Thread Eric Liu via Phabricator via cfe-commits 
ioeric added a comment.

In https://reviews.llvm.org/D52078#1238301, @sammccall wrote:

> Something not listed in cons: because macros aren't namespaced and we don't 
> have lots of signals, they can be really spammy. 
>  Potentially, offering macros that aren't in the TU could be a loss even if 
> it's a win for other types of signals.


Aren't they already spammy from Sema? Sema can provide thousands of macros in 
the TU.

We penalize quality of macro symbols in the global index. Maybe we can do the 
same thing for dynamic index?

> We could always e.g. postfilter index macro results using the include 
> structure of the preamble, so no concern for this patch, just something to 
> think about for the followup.

Sounds good.

> We also need to make sure that we're not indexing/serving header guards as 
> code completions (e.g. if SemaCodeComplete is currently taking care of this)

These symbols are already filtered out in `SymbolCollector`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078



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

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-18 Thread Sam McCall via Phabricator via cfe-commits 
sammccall accepted this revision.
sammccall added a comment.

Something not listed in cons: because macros aren't namespaced and we don't 
have lots of signals, they can be really spammy. 
Potentially, offering macros that aren't in the TU could be a loss even if it's 
a win for other types of signals.

We could always e.g. postfilter index macro results using the include structure 
of the preamble, so no concern for this patch, just something to think about 
for the followup.

We also need to make sure that we're not indexing/serving header guards as code 
completions (e.g. if SemaCodeComplete is currently taking care of this)




Comment at: clangd/index/FileIndex.cpp:48
 CollectorOpts.RefFilter = RefKind::All;
+  }else {
+IndexOpts.IndexMacrosInPreprocessor = true;

nit: please clang-format


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078



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

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-18 Thread Eric Liu via Phabricator via cfe-commits 
ioeric updated this revision to Diff 165959.
ioeric added a comment.

- merge with origin/master


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078

Files:
  clangd/index/FileIndex.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp

Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -15,6 +15,7 @@
 #include "index/FileIndex.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Index/IndexSymbol.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "gtest/gtest.h"
@@ -330,6 +331,21 @@
  FileURI("unittest:///test2.cc"))}));
 }
 
+TEST(FileIndexTest, CollectMacros) {
+  FileIndex M;
+  update(M, "f", "#define CLANGD 1");
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  bool SeenSymbol = false;
+  M.index().fuzzyFind(Req, [&](const Symbol ) {
+EXPECT_EQ(Sym.Name, "CLANGD");
+EXPECT_EQ(Sym.SymInfo.Kind, index::SymbolKind::Macro);
+SeenSymbol = true;
+  });
+  EXPECT_TRUE(SeenSymbol);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -223,12 +223,13 @@
 void TestAfterDotCompletion(clangd::CodeCompleteOptions Opts) {
   auto Results = completions(
   R"cpp(
-  #define MACRO X
-
   int global_var;
 
   int global_func();
 
+  // Make sure this is not in preamble.
+  #define MACRO X
+
   struct GlobalClass {};
 
   struct ClassWithMembers {
@@ -276,11 +277,12 @@
 void TestGlobalScopeCompletion(clangd::CodeCompleteOptions Opts) {
   auto Results = completions(
   R"cpp(
-  #define MACRO X
-
   int global_var;
   int global_func();
 
+  // Make sure this is not in preamble.
+  #define MACRO X
+
   struct GlobalClass {};
 
   struct ClassWithMembers {
@@ -430,10 +432,11 @@
 TEST(CompletionTest, Kinds) {
   auto Results = completions(
   R"cpp(
-  #define MACRO X
   int variable;
   struct Struct {};
   int function();
+  // make sure MACRO is not included in preamble.
+  #define MACRO 10
   int X = ^
   )cpp",
   {func("indexFunction"), var("indexVariable"), cls("indexClass")});
@@ -1921,6 +1924,21 @@
   UnorderedElementsAre(Named("Clangd_Macro_Test")));
 }
 
+TEST(CompletionTest, NoMacroFromPreambleIfIndexIsSet) {
+  auto Results = completions(
+  R"cpp(#define CLANGD_PREAMBLE x
+
+  int x = 0;
+  #define CLANGD_MAIN x
+  void f() { CLANGD_^ }
+  )cpp",
+  {func("CLANGD_INDEX")});
+  // Index is overriden in code completion options, so the preamble symbol is
+  // not seen.
+  EXPECT_THAT(Results.Completions, UnorderedElementsAre(Named("CLANGD_MAIN"),
+Named("CLANGD_INDEX")));
+}
+
 TEST(CompletionTest, DeprecatedResults) {
   std::string Body = R"cpp(
 void TestClangd();
Index: clangd/index/FileIndex.cpp
===
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -14,6 +14,7 @@
 #include "index/Index.h"
 #include "index/Merge.h"
 #include "clang/Index/IndexingAction.h"
+#include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include 
 
@@ -41,10 +42,13 @@
   IndexOpts.SystemSymbolFilter =
   index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly;
   IndexOpts.IndexFunctionLocals = false;
-
-  if (IsIndexMainAST)
+  if (IsIndexMainAST) {
 // We only collect refs when indexing main AST.
 CollectorOpts.RefFilter = RefKind::All;
+  }else {
+IndexOpts.IndexMacrosInPreprocessor = true;
+CollectorOpts.CollectMacro = true;
+  }
 
   SymbolCollector Collector(std::move(CollectorOpts));
   Collector.setPreprocessor(PP);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-15 Thread Sam McCall via cfe-commits 
On Sat, Sep 15, 2018, 08:36 Eric Liu via Phabricator <
revi...@reviews.llvm.org> wrote:

> ioeric added inline comments.
>
>
> 
> Comment at: clangd/index/FileIndex.h:93
>  std::pair
>  indexAST(ASTContext , std::shared_ptr PP,
> + bool IndexMacros = false,
> 
> sammccall wrote:
> > I'm concerned about the proliferation of parameters here. (Not new with
> this patch, it's been a continuous process - the first version had one
> input and one output!)
> > It's heading in the direction of being a catch-all "collect some data
> from an AST" function, at which point each caller has to think hard about
> every option and we're going to end up with bugs.
> > (For example: TestTU::index() passes "false" for IndexMacros. It seems
> to me maybe it should be "true". But it's really hard to tell.)
> > That's also pretty similar to the mission of SymbolCollector itself, so
> we're going to get some confusion there.
> >
> > As far as I can tell, there's now two types of indexing that we do:
> >   - preamble indexing: we look at all decls, and only index those
> outside the main file. We index macros. We don't collect references. Inputs
> are ASTContext and PP.
> >   - mainfile-style indexing: we look only at top-level decls in the main
> file. We don't index macros. We collect references from the main file.
> Inputs are ParsedAST.
> > This really looks like it should be 2 functions with 2 and 1 parameters,
> rather than 1 function with 4.
> > Then callers will have two styles of indexing (with names!) to choose
> between, rather than being required to design their own. And we can get rid
> of the "is this a main-file index?" hacks in the implementation.
> >
> > Sorry for jumping on you here, this change isn't any worse than the
> three previous patches that added knobs to this function.
> > This doesn't need to be addressed in this patch - could be before or
> after, and I'm happy to take this on myself. But I think we shouldn't kick
> this can down the road too much further, eventually we end up with APIs
> like clang :-)
> I'm definitely on board with this and happy to do the refactoring before
> landing this patch, to break API just once ;)  Just to clarify my
> understanding before doing that, do we also want to split
> `FileIndex::update` into `updatePreamble` and `updateMain`?  And the new
> `indexAST` will be `indexMain` and `indexPreamble`?
>
Basically yes, though it's a bit weird that each instance would use one
method or the other.

> I think if we're going to break the API, we should break it good :-)

FileIndex is pretty thin today logic-wise, all the opinions are in
FileSymbols, indexAST, and the DynamicIndex callers. I think it could stand
to take on more responsibility.
What if we merged DynamicIndex into it? So it'd have both updatePreamble
and updateMain, but they would update two separate FileSymbols. FileIndex
would maintain two SwapIndexes, and expose a merged index.

This would take the logic out of ClangdServer (where it's hard to test) and
also I think we have an out-of-tree DynamicIndex copy we could remove.
And I think the API would push us towards sensible decisions (e.g. I think
the answer for testTU::index() is it really wants to be a one-file
FileIndex with both indexing types)

Thoughts?


>
> Repository:
>   rCTE Clang Tools Extra
>
> https://reviews.llvm.org/D52078
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-15 Thread Eric Liu via Phabricator via cfe-commits 
ioeric added inline comments.



Comment at: clangd/index/FileIndex.h:93
 std::pair
 indexAST(ASTContext , std::shared_ptr PP,
+ bool IndexMacros = false,

sammccall wrote:
> I'm concerned about the proliferation of parameters here. (Not new with this 
> patch, it's been a continuous process - the first version had one input and 
> one output!)
> It's heading in the direction of being a catch-all "collect some data from an 
> AST" function, at which point each caller has to think hard about every 
> option and we're going to end up with bugs.
> (For example: TestTU::index() passes "false" for IndexMacros. It seems to me 
> maybe it should be "true". But it's really hard to tell.)
> That's also pretty similar to the mission of SymbolCollector itself, so we're 
> going to get some confusion there.
> 
> As far as I can tell, there's now two types of indexing that we do:
>   - preamble indexing: we look at all decls, and only index those outside the 
> main file. We index macros. We don't collect references. Inputs are 
> ASTContext and PP.
>   - mainfile-style indexing: we look only at top-level decls in the main 
> file. We don't index macros. We collect references from the main file. Inputs 
> are ParsedAST.
> This really looks like it should be 2 functions with 2 and 1 parameters, 
> rather than 1 function with 4.
> Then callers will have two styles of indexing (with names!) to choose 
> between, rather than being required to design their own. And we can get rid 
> of the "is this a main-file index?" hacks in the implementation.
> 
> Sorry for jumping on you here, this change isn't any worse than the three 
> previous patches that added knobs to this function.
> This doesn't need to be addressed in this patch - could be before or after, 
> and I'm happy to take this on myself. But I think we shouldn't kick this can 
> down the road too much further, eventually we end up with APIs like clang :-)
I'm definitely on board with this and happy to do the refactoring before 
landing this patch, to break API just once ;)  Just to clarify my understanding 
before doing that, do we also want to split `FileIndex::update` into 
`updatePreamble` and `updateMain`?  And the new `indexAST` will be `indexMain` 
and `indexPreamble`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078



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

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-14 Thread Sam McCall via Phabricator via cfe-commits 
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This is definitely the right thing to do, thanks for finding it!
I've got a long comment about how everything used to be simpler in the good old 
days :-) I'm itching to refactor a bit, but land this first.




Comment at: clangd/index/FileIndex.cpp:34
   CollectorOpts.CountReferences = false;
+  CollectorOpts.CollectMacro = true;
   if (!URISchemes.empty())

I don't understand this change. Don't we also want this only if IndexMacros is 
true?
(It's possible it doesn't make a difference due to implementation details but 
it seems worth being clear here)



Comment at: clangd/index/FileIndex.h:93
 std::pair
 indexAST(ASTContext , std::shared_ptr PP,
+ bool IndexMacros = false,

I'm concerned about the proliferation of parameters here. (Not new with this 
patch, it's been a continuous process - the first version had one input and one 
output!)
It's heading in the direction of being a catch-all "collect some data from an 
AST" function, at which point each caller has to think hard about every option 
and we're going to end up with bugs.
(For example: TestTU::index() passes "false" for IndexMacros. It seems to me 
maybe it should be "true". But it's really hard to tell.)
That's also pretty similar to the mission of SymbolCollector itself, so we're 
going to get some confusion there.

As far as I can tell, there's now two types of indexing that we do:
  - preamble indexing: we look at all decls, and only index those outside the 
main file. We index macros. We don't collect references. Inputs are ASTContext 
and PP.
  - mainfile-style indexing: we look only at top-level decls in the main file. 
We don't index macros. We collect references from the main file. Inputs are 
ParsedAST.
This really looks like it should be 2 functions with 2 and 1 parameters, rather 
than 1 function with 4.
Then callers will have two styles of indexing (with names!) to choose between, 
rather than being required to design their own. And we can get rid of the "is 
this a main-file index?" hacks in the implementation.

Sorry for jumping on you here, this change isn't any worse than the three 
previous patches that added knobs to this function.
This doesn't need to be addressed in this patch - could be before or after, and 
I'm happy to take this on myself. But I think we shouldn't kick this can down 
the road too much further, eventually we end up with APIs like clang :-)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078



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

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-14 Thread Eric Liu via Phabricator via cfe-commits 
ioeric added a comment.

In https://reviews.llvm.org/D52078#1234667, @ilya-biryukov wrote:

> ~1% increase in memory usage seems totally fine. Actually surprised it's that 
> small.


Tested on a larger file: it's ~5% for `ClangdServer.cpp`. I think it's still 
worth it for the speedup.




Comment at: clangd/index/FileIndex.cpp:84
+Merged.insert(Macro);
+  for (const auto  : Collector.takeSymbols())
+Merged.insert(Sym);

ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > Why make an extra copy of the symbols? Why not add macros to the same 
> > > builder used in collector?
> > `index::indexTopLevelDecls` will re-`initialize` the collector. This is 
> > safe with the current implementation, but I can imagine it goes wrong when 
> > we do more cleanup in the initialization.
> Sorry, missed the comment about it.
> And if we do this after `indexTopLevelDecls`, than we'll be calling after the 
> `finish` call. **Sigh**...
> 
> Doing an extra copy of the symbols here is wasteful and makes the code more 
> complicated than it should be.
> 
> We have alternatives that could fix this:
> 1. Move `IndexMacro` logic into `index::indexTopLevelDecls`
> 2. Move macro collection to `SymbolCollector::finish()`
> 3. Extract a function that creates a symbol for macro definition, so we don't 
> need to call `handleMacroOccurence` and add can add extra macro symbols after 
> `SymbolCollector` finishes.
> 
> 
> (1) seems to be the cleanest option, WDYT?
(1) sounds good. D52098


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078



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

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-14 Thread Eric Liu via Phabricator via cfe-commits 
ioeric updated this revision to Diff 165517.
ioeric added a comment.

- Move macro collection to indexTopLevelDecls.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078

Files:
  clangd/ClangdServer.cpp
  clangd/FindSymbols.cpp
  clangd/XRefs.cpp
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/TestTU.cpp

Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -51,7 +51,7 @@
 std::unique_ptr TestTU::index() const {
   auto AST = build();
   auto Content = indexAST(AST.getASTContext(), AST.getPreprocessorPtr(),
-  AST.getLocalTopLevelDecls());
+  /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
   return MemIndex::build(std::move(Content.first), std::move(Content.second));
 }
 
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -244,7 +244,7 @@
   Test.Filename = "test.cc";
   auto AST = Test.build();
   Dyn.update(Test.Filename, (), AST.getPreprocessorPtr(),
- AST.getLocalTopLevelDecls());
+ /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
 
   // Build static index for test.cc.
   Test.HeaderCode = HeaderCode;
@@ -254,7 +254,7 @@
   // Add stale refs for test.cc.
   StaticIndex.update(Test.Filename, (),
  StaticAST.getPreprocessorPtr(),
- StaticAST.getLocalTopLevelDecls());
+ /*IndexMacros=*/false, StaticAST.getLocalTopLevelDecls());
 
   // Add refs for test2.cc
   Annotations Test2Code(R"(class $Foo[[Foo]] {};)");
@@ -265,7 +265,7 @@
   StaticAST = Test2.build();
   StaticIndex.update(Test2.Filename, (),
  StaticAST.getPreprocessorPtr(),
- StaticAST.getLocalTopLevelDecls());
+ /*IndexMacros=*/false, StaticAST.getLocalTopLevelDecls());
 
   RefsRequest Request;
   Request.IDs = {Foo.ID};
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -15,6 +15,7 @@
 #include "index/FileIndex.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Index/IndexSymbol.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "gtest/gtest.h"
@@ -135,7 +136,8 @@
   File.HeaderFilename = (Basename + ".h").str();
   File.HeaderCode = Code;
   auto AST = File.build();
-  M.update(File.Filename, (), AST.getPreprocessorPtr());
+  M.update(File.Filename, (), AST.getPreprocessorPtr(),
+   /*IndexMacros=*/true);
 }
 
 TEST(FileIndexTest, CustomizedURIScheme) {
@@ -333,23 +335,38 @@
   Test.Filename = "test.cc";
   auto AST = Test.build();
   Index.update(Test.Filename, (), AST.getPreprocessorPtr(),
-   AST.getLocalTopLevelDecls());
+   /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
   // Add test2.cc
   TestTU Test2;
   Test2.HeaderCode = HeaderCode;
   Test2.Code = MainCode.code();
   Test2.Filename = "test2.cc";
   AST = Test2.build();
   Index.update(Test2.Filename, (), AST.getPreprocessorPtr(),
-   AST.getLocalTopLevelDecls());
+   /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
 
   EXPECT_THAT(getRefs(Index, Foo.ID),
   RefsAre({AllOf(RefRange(MainCode.range("foo")),
  FileURI("unittest:///test.cc")),
AllOf(RefRange(MainCode.range("foo")),
  FileURI("unittest:///test2.cc"))}));
 }
 
+TEST(FileIndexTest, CollectMacros) {
+  FileIndex M;
+  update(M, "f", "#define CLANGD 1");
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  bool SeenSymbol = false;
+  M.fuzzyFind(Req, [&](const Symbol ) {
+EXPECT_EQ(Sym.Name, "CLANGD");
+EXPECT_EQ(Sym.SymInfo.Kind, index::SymbolKind::Macro);
+SeenSymbol = true;
+  });
+  EXPECT_TRUE(SeenSymbol);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -223,12 +223,13 @@
 void TestAfterDotCompletion(clangd::CodeCompleteOptions Opts) {
   auto Results = completions(
   R"cpp(
-  #define MACRO X
-
   int global_var;
 
   int global_func();
 
+  // Make sure this is not in preamble.
+  #define MACRO X
+
   struct GlobalClass {};
 
   struct ClassWithMembers {
@@ -276,11 +277,12 @@
 void TestGlobalScopeCompletion(clangd::CodeCompleteOptions Opts) {

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-14 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added inline comments.



Comment at: clangd/index/FileIndex.cpp:84
+Merged.insert(Macro);
+  for (const auto  : Collector.takeSymbols())
+Merged.insert(Sym);

ioeric wrote:
> ilya-biryukov wrote:
> > Why make an extra copy of the symbols? Why not add macros to the same 
> > builder used in collector?
> `index::indexTopLevelDecls` will re-`initialize` the collector. This is safe 
> with the current implementation, but I can imagine it goes wrong when we do 
> more cleanup in the initialization.
Sorry, missed the comment about it.
And if we do this after `indexTopLevelDecls`, than we'll be calling after the 
`finish` call. **Sigh**...

Doing an extra copy of the symbols here is wasteful and makes the code more 
complicated than it should be.

We have alternatives that could fix this:
1. Move `IndexMacro` logic into `index::indexTopLevelDecls`
2. Move macro collection to `SymbolCollector::finish()`
3. Extract a function that creates a symbol for macro definition, so we don't 
need to call `handleMacroOccurence` and add can add extra macro symbols after 
`SymbolCollector` finishes.


(1) seems to be the cleanest option, WDYT?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078



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

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-14 Thread Eric Liu via Phabricator via cfe-commits 
ioeric added inline comments.



Comment at: clangd/index/FileIndex.cpp:84
+Merged.insert(Macro);
+  for (const auto  : Collector.takeSymbols())
+Merged.insert(Sym);

ilya-biryukov wrote:
> Why make an extra copy of the symbols? Why not add macros to the same builder 
> used in collector?
`index::indexTopLevelDecls` will re-`initialize` the collector. This is safe 
with the current implementation, but I can imagine it goes wrong when we do 
more cleanup in the initialization.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078



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

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-14 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

~1% increase in memory usage seems totally fine. Actually surprised it's that 
small.
Overall LG, just a single comment about extra copying. Thanks for the change, 
looks like a great improvement!




Comment at: clangd/index/FileIndex.cpp:84
+Merged.insert(Macro);
+  for (const auto  : Collector.takeSymbols())
+Merged.insert(Sym);

Why make an extra copy of the symbols? Why not add macros to the same builder 
used in collector?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078



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

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-14 Thread Eric Liu via Phabricator via cfe-commits 
ioeric updated this revision to Diff 165444.
ioeric added a comment.

- minor cleanup


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078

Files:
  clangd/ClangdServer.cpp
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/TestTU.cpp

Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -51,7 +51,7 @@
 std::unique_ptr TestTU::index() const {
   auto AST = build();
   auto Content = indexAST(AST.getASTContext(), AST.getPreprocessorPtr(),
-  AST.getLocalTopLevelDecls());
+  /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
   return MemIndex::build(std::move(Content.first), std::move(Content.second));
 }
 
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -244,7 +244,7 @@
   Test.Filename = "test.cc";
   auto AST = Test.build();
   Dyn.update(Test.Filename, (), AST.getPreprocessorPtr(),
- AST.getLocalTopLevelDecls());
+ /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
 
   // Build static index for test.cc.
   Test.HeaderCode = HeaderCode;
@@ -254,7 +254,7 @@
   // Add stale refs for test.cc.
   StaticIndex.update(Test.Filename, (),
  StaticAST.getPreprocessorPtr(),
- StaticAST.getLocalTopLevelDecls());
+ /*IndexMacros=*/false, StaticAST.getLocalTopLevelDecls());
 
   // Add refs for test2.cc
   Annotations Test2Code(R"(class $Foo[[Foo]] {};)");
@@ -265,7 +265,7 @@
   StaticAST = Test2.build();
   StaticIndex.update(Test2.Filename, (),
  StaticAST.getPreprocessorPtr(),
- StaticAST.getLocalTopLevelDecls());
+ /*IndexMacros=*/false, StaticAST.getLocalTopLevelDecls());
 
   RefsRequest Request;
   Request.IDs = {Foo.ID};
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -15,6 +15,7 @@
 #include "index/FileIndex.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Index/IndexSymbol.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "gtest/gtest.h"
@@ -135,7 +136,8 @@
   File.HeaderFilename = (Basename + ".h").str();
   File.HeaderCode = Code;
   auto AST = File.build();
-  M.update(File.Filename, (), AST.getPreprocessorPtr());
+  M.update(File.Filename, (), AST.getPreprocessorPtr(),
+   /*IndexMacros=*/true);
 }
 
 TEST(FileIndexTest, CustomizedURIScheme) {
@@ -333,23 +335,38 @@
   Test.Filename = "test.cc";
   auto AST = Test.build();
   Index.update(Test.Filename, (), AST.getPreprocessorPtr(),
-   AST.getLocalTopLevelDecls());
+   /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
   // Add test2.cc
   TestTU Test2;
   Test2.HeaderCode = HeaderCode;
   Test2.Code = MainCode.code();
   Test2.Filename = "test2.cc";
   AST = Test2.build();
   Index.update(Test2.Filename, (), AST.getPreprocessorPtr(),
-   AST.getLocalTopLevelDecls());
+   /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
 
   EXPECT_THAT(getRefs(Index, Foo.ID),
   RefsAre({AllOf(RefRange(MainCode.range("foo")),
  FileURI("unittest:///test.cc")),
AllOf(RefRange(MainCode.range("foo")),
  FileURI("unittest:///test2.cc"))}));
 }
 
+TEST(FileIndexTest, CollectMacros) {
+  FileIndex M;
+  update(M, "f", "#define CLANGD 1");
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  bool SeenSymbol = false;
+  M.fuzzyFind(Req, [&](const Symbol ) {
+EXPECT_EQ(Sym.Name, "CLANGD");
+EXPECT_EQ(Sym.SymInfo.Kind, index::SymbolKind::Macro);
+SeenSymbol = true;
+  });
+  EXPECT_TRUE(SeenSymbol);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -223,12 +223,13 @@
 void TestAfterDotCompletion(clangd::CodeCompleteOptions Opts) {
   auto Results = completions(
   R"cpp(
-  #define MACRO X
-
   int global_var;
 
   int global_func();
 
+  // Make sure this is not in preamble.
+  #define MACRO X
+
   struct GlobalClass {};
 
   struct ClassWithMembers {
@@ -276,11 +277,12 @@
 void TestGlobalScopeCompletion(clangd::CodeCompleteOptions Opts) {
   auto Results = completions(
   R"cpp(
-  #define MACRO X
-
   int

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-14 Thread Eric Liu via Phabricator via cfe-commits 
ioeric created this revision.
ioeric added reviewers: ilya-biryukov, sammccall.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay.

Pros:
o Loading macros from preamble for every completion is slow (see profile).
o Calculating macro USR is also slow (see profile).
o Sema can provide a lot of macro completion results (e.g. when filter is empty,
60k for some large TUs!).

Cons:
o Slight memory increase in dynamic index (~1%).
o Some extra work during preamble build (should be fine as preamble build and
indexAST is way slower).

[profile place holder]


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078

Files:
  clangd/ClangdServer.cpp
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/TestTU.cpp

Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -51,7 +51,7 @@
 std::unique_ptr TestTU::index() const {
   auto AST = build();
   auto Content = indexAST(AST.getASTContext(), AST.getPreprocessorPtr(),
-  AST.getLocalTopLevelDecls());
+  /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
   return MemIndex::build(std::move(Content.first), std::move(Content.second));
 }
 
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -244,7 +244,7 @@
   Test.Filename = "test.cc";
   auto AST = Test.build();
   Dyn.update(Test.Filename, (), AST.getPreprocessorPtr(),
- AST.getLocalTopLevelDecls());
+ /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
 
   // Build static index for test.cc.
   Test.HeaderCode = HeaderCode;
@@ -254,7 +254,7 @@
   // Add stale refs for test.cc.
   StaticIndex.update(Test.Filename, (),
  StaticAST.getPreprocessorPtr(),
- StaticAST.getLocalTopLevelDecls());
+ /*IndexMacros=*/false, StaticAST.getLocalTopLevelDecls());
 
   // Add refs for test2.cc
   Annotations Test2Code(R"(class $Foo[[Foo]] {};)");
@@ -265,7 +265,7 @@
   StaticAST = Test2.build();
   StaticIndex.update(Test2.Filename, (),
  StaticAST.getPreprocessorPtr(),
- StaticAST.getLocalTopLevelDecls());
+ /*IndexMacros=*/false, StaticAST.getLocalTopLevelDecls());
 
   RefsRequest Request;
   Request.IDs = {Foo.ID};
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -15,6 +15,7 @@
 #include "index/FileIndex.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
+#include "clang/Index/IndexSymbol.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "gtest/gtest.h"
@@ -135,7 +136,8 @@
   File.HeaderFilename = (Basename + ".h").str();
   File.HeaderCode = Code;
   auto AST = File.build();
-  M.update(File.Filename, (), AST.getPreprocessorPtr());
+  M.update(File.Filename, (), AST.getPreprocessorPtr(),
+   /*IndexMacros=*/true);
 }
 
 TEST(FileIndexTest, CustomizedURIScheme) {
@@ -333,23 +335,38 @@
   Test.Filename = "test.cc";
   auto AST = Test.build();
   Index.update(Test.Filename, (), AST.getPreprocessorPtr(),
-   AST.getLocalTopLevelDecls());
+   /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
   // Add test2.cc
   TestTU Test2;
   Test2.HeaderCode = HeaderCode;
   Test2.Code = MainCode.code();
   Test2.Filename = "test2.cc";
   AST = Test2.build();
   Index.update(Test2.Filename, (), AST.getPreprocessorPtr(),
-   AST.getLocalTopLevelDecls());
+   /*IndexMacros=*/false, AST.getLocalTopLevelDecls());
 
   EXPECT_THAT(getRefs(Index, Foo.ID),
   RefsAre({AllOf(RefRange(MainCode.range("foo")),
  FileURI("unittest:///test.cc")),
AllOf(RefRange(MainCode.range("foo")),
  FileURI("unittest:///test2.cc"))}));
 }
 
+TEST(FileIndexTest, CollectMacros) {
+  FileIndex M;
+  update(M, "f", "#define MAC 1; class X {};");
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  bool SeenSymbol = false;
+  M.fuzzyFind(Req, [&](const Symbol ) {
+EXPECT_EQ(Sym.Name, "MAC");
+EXPECT_EQ(Sym.SymInfo.Kind, index::SymbolKind::Macro);
+SeenSymbol = true;
+  });
+  EXPECT_TRUE(SeenSymbol);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -223,12