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

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&id=166078#toc Repository: rCTE Clang

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

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-b

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

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

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

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 >

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

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 sign

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

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: unitte

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

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 &AST, std::shared_ptr PP, > + bool IndexMacros = false, > ---

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

ioeric added inline comments. Comment at: clangd/index/FileIndex.h:93 std::pair indexAST(ASTContext &AST, std::shared_ptr PP, + bool IndexMacros = false, sammccall wrote: > I'm concerned about the proliferation of parameters here. (Not new with this >

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

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, bu

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

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.

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

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 c

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

ilya-biryukov added inline comments. Comment at: clangd/index/FileIndex.cpp:84 +Merged.insert(Macro); + for (const auto &Sym : Collector.takeSymbols()) +Merged.insert(Sym); ioeric wrote: > ilya-biryukov wrote: > > Why make an extra copy of the symbols? W

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

ioeric added inline comments. Comment at: clangd/index/FileIndex.cpp:84 +Merged.insert(Macro); + for (const auto &Sym : Collector.takeSymbols()) +Merged.insert(Sym); ilya-biryukov wrote: > Why make an extra copy of the symbols? Why not add macros to the

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

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 +Mer

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

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 unittest

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

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 c