[PATCH] D47935: [clangd] Boost completion score according to file proximity.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks! Just nits Comment at: clangd/Quality.cpp:304 OS << formatv("\tForbidden: {0}\n", S.Forbidden); - OS << formatv("\tProximity: {0}\n", S.ProximityScore); + O

[PATCH] D47931: [clangd] Customizable URI schemes for dynamic index.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: unittests/clangd/FileIndexTests.cpp:99 +class TestURIScheme : public URIScheme { +public: This can use the shared unittest: scheme fr

[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.

sammccall marked 4 inline comments as done. sammccall added inline comments. Comment at: clangd/CodeComplete.cpp:245 +// Methods are simply grouped by name. +return hash_combine('M', IndexResult->Name); + case index::SymbolKind::Function: ily

[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.

sammccall updated this revision to Diff 151476. sammccall marked an inline comment as done. sammccall added a comment. Review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D47957 Files: clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/tool/ClangdMain.cpp un

[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rCTE334822: [clangd] Add option to fold overloads into a single completion item. (authored by sammccall, committed by ). Changed prior to commit: https://review

[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/CodeComplete.cpp:301 + } else if (!IncludePath->empty()) { +if (auto Edit = Includes.insert(*IncludePath)) { +

[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.

sammccall added inline comments. Comment at: clangd/CodeComplete.cpp:313 +I.label = +(InsertingInclude ? Opts.IncludeInsertionIndicator : " ") + I.label; I.scoreInfo = Scores; sammccall wrote: > string(IncludeInsertionIndicator.size(), ' ')? oops

[PATCH] D48211: [clangd] Do not show namespace comments.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/CodeCompletionStrings.cpp:172 // get this declaration, so we don't show documentation in that case. if (Result.Kind != CodeCompletionResult

[PATCH] D48290: [clangd] Use workspace root path as hint path for resolving URIs in workspace/symbol

sammccall added inline comments. Comment at: clangd/ClangdServer.cpp:119 + auto FS = FSProvider.getFileSystem(); + auto Status = FS->status(RootPath); + if (!Status) why validate it? Comment at: clangd/FindSymbols.h:30 /// \p Limit limits t

[PATCH] D48290: [clangd] Use workspace root path as hint path for resolving URIs in workspace/symbol

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: unittests/clangd/TestFS.cpp:93 return URI(Scheme, /*Authority=*/"", - llvm::sys::path::convert_to_slash(Body)); + Strin

[PATCH] D48368: [clangd] Sema ranking tweaks: downrank keywords and injected names.

sammccall created this revision. Herald added subscribers: cfe-commits, jkorous, MaskRay, ioeric, ilya-biryukov. Injected names being ranked too high was just a bug. The high boost for keywords was intended, but was too much given how useless keywords are. We should probably boost them on a case-b

[PATCH] D48375: [clangd] Remove FilterText from the index.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, jkorous, MaskRay, ilya-biryukov. It's almost always identical to Name, and in fact we never used it (we used name instead). The only case where they differ is objc method selectors (foo: vs

[PATCH] D48375: [clangd] Remove FilterText from the index.

sammccall updated this revision to Diff 152108. sammccall added a comment. use foo: instead of foo:bar: for name. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48375 Files: clangd/CodeComplete.cpp clangd/CodeCompletionStrings.cpp clangd/CodeCompletionStrings.h clangd/i

[PATCH] D48418: [clangd] Expose 'shouldCollectSymbol' helper from SymbolCollector.

sammccall added a comment. Can you motivate this change a bit, and add tests? Comment at: clangd/index/SymbolCollector.h:61 + /// AST matchers require non-const ASTContext. + static bool shouldCollectSymbol(const NamedDecl &ND, ASTContext &ASTCtx); + in princ

[PATCH] D48425: [clangd] Expose qualified symbol names in CompletionItem (C++ structure only, no json).

sammccall added inline comments. Comment at: clangd/CodeComplete.cpp:246 assert(bool(SemaResult) == bool(SemaCCS)); +assert(bool(SemaResult) || bool(IndexResult)); + can skip the bool() here if you like Comment at: clangd/Protocol.h:74

[PATCH] D48441: [clangd] Incorporate transitive #includes into code complete proximity scoring.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, jkorous, MaskRay, ilya-biryukov, mgorny. We now compute a distance from the main file to the symbol header, which is a weighted count of: - some number of #include traversals from source fi

[PATCH] D48441: [clangd] Incorporate transitive #includes into code complete proximity scoring.

sammccall updated this revision to Diff 152326. sammccall added a comment. Document some more stuff, and clang-format Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48441 Files: clangd/CMakeLists.txt clangd/ClangdServer.cpp clangd/ClangdUnit.cpp clangd/ClangdUnit.h cl

[PATCH] D48375: [clangd] Remove FilterText from the index.

This revision was automatically updated to reflect the committed changes. Closed by commit rL335321: [clangd] Remove FilterText from the index. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D48375 Files: clang-t

[PATCH] D48475: [clangd] More precise representation of symbol names/labels in the index.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, jkorous, MaskRay, ilya-biryukov. Previously, the strings matched LSP completion pretty closely. The completion label was a single string, for instance. This made implementing completion itse

[PATCH] D48425: [clangd] Expose qualified symbol names in CompletionItem (C++ structure only, no json).

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/SourceCode.h:60 +/// Splits the qualified name of ND. The scope doesn't contain unwritten scopes +/// like inline namespaces.

[PATCH] D48441: [clangd] Incorporate transitive #includes into code complete proximity scoring.

sammccall updated this revision to Diff 152479. sammccall marked 9 inline comments as done. sammccall added a comment. Address review comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48441 Files: clangd/CMakeLists.txt clangd/ClangdServer.cpp clangd/ClangdUnit.cpp

[PATCH] D48475: [clangd] More precise representation of symbol names/labels in the index.

sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clangd/CodeCompletionStrings.cpp:122 + // Typed-text chunk is the actual name. Text before it is qualifiers. + if (Qualifiers) +*Qualifiers = std::move(*Signature);

[PATCH] D48475: [clangd] More precise representation of symbol names/labels in the index.

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rCTE335360: [clangd] More precise representation of symbol names/labels in the index. (authored by sammccall, committed by ). Changed prior to commit: https://r

[PATCH] D48492: [clang-format] Add a default format style that can be used by users of `getStyle`

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. I think this should work great for us, and hopefully helps other downstream users too. (@djasper: the plan is to introduce a new style name for our tweaked version of `"file"`, and make

[PATCH] D53019: [clangd] dump xrefs information in dexp tool.

sammccall added inline comments. Comment at: clangd/index/dex/dexp/Dexp.cpp:172 + "filter", cl::init(".*"), + cl::desc("Print all results from files matching this filter."), + }; filter -> regular expression Comment at: clangd/index/

[PATCH] D53019: [clangd] dump xrefs information in dexp tool.

sammccall added inline comments. Comment at: clangd/index/dex/dexp/Dexp.cpp:65 + clang::clangd::SymbolID SymID; + // We choose the first one if there are overloaded symbols. + Index->fuzzyFind(Request, [&](const Symbol &Sym) { I don't think this is reasonable

[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, jfb, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov, mgorny. See tinyurl.com/clangd-automatic-index for design and goals. Lots of limitations to keep this patch smallish, TODOs everyw

[PATCH] D53034: [clangd] Remove no-op crash handler, we never set a crash context.

sammccall created this revision. sammccall added reviewers: ioeric, ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay. I think this was just copied from somewhere with the belief that it actually did some crash handling. Of course the question arises: *sho

[PATCH] D53070: [CodeComplete] Fix crash when completing params function declarations.

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added a subscriber: cfe-commits. In a decl like `int AA(BB cc)` where BB isn't defined, we end up trying to parse `BB cc` as an expression (vexing parse) and end up triggering the parser's "recovery-in-function" completi

[PATCH] D53070: [CodeComplete] Fix crash when completing params function declarations.

This revision was automatically updated to reflect the committed changes. Closed by commit rC344133: [CodeComplete] Fix crash when completing params function declarations. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53070?vs=168967&id=168974#toc R

[PATCH] D53135: Remove top-level using declaration from header files, as these aliases leak.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, arphaman. Repository: rC Clang https://reviews.llvm.org/D53135 Files: include/clang/Driver/Job.h include/clang/Frontend/PCHContainerOperations.h include/clang/Serialization/

[PATCH] D53131: [clangd] Support scope proximity in code completion.

sammccall added inline comments. Comment at: clangd/CodeComplete.cpp:558 +if (const auto *NS = dyn_cast(Ctx)) + return NS->getQualifiedNameAsString() + "::"; + return llvm::None; does this do the right thing if it's anonymous or inline, or a parent is?

[PATCH] D53034: [clangd] Remove no-op crash handler, we never set a crash context.

This revision was automatically updated to reflect the committed changes. Closed by commit rL344245: [clangd] Remove no-op crash handler, we never set a crash context. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/

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

sammccall added a comment. In https://reviews.llvm.org/D51725#1261798, @ilya-biryukov wrote: > In https://reviews.llvm.org/D51725#1255695, @simark wrote: > > > But I am wondering what to do with the feature that allows clangd to change > > compilation database path using the `didChangeConfigurat

[PATCH] D53145: [Tooling] Support src/buildroot to find the build-root, and expose it.

sammccall created this revision. Herald added a subscriber: cfe-commits. Prototype implementation, no tests yet. See http://lists.llvm.org/pipermail/cfe-dev/2018-October/059752.html Repository: rC Clang https://reviews.llvm.org/D53145 Files: include/clang/Tooling/CompilationDatabase.h in

[PATCH] D53135: Remove top-level using declaration from header files, as these aliases leak.

sammccall added inline comments. Comment at: include/clang/Driver/Job.h:33 // Re-export this as clang::driver::ArgStringList. -using llvm::opt::ArgStringList; +using ArgStringList = llvm::opt::ArgStringList; ilya-biryukov wrote: > Both approaches seem equivale

[PATCH] D53131: [clangd] Support scope proximity in code completion.

sammccall added a comment. (BTW I wouldn't expect to see improvements on eval here, as Comment at: clangd/CodeComplete.cpp:558 +if (const auto *NS = dyn_cast(Ctx)) + return NS->getQualifiedNameAsString() + "::"; + return llvm::None; ioeric wrote: > sa

[PATCH] D53191: [clang] Use Statement and Namespace instead of Name and PotentiallyQualifiedName

sammccall added inline comments. Comment at: lib/Sema/SemaCodeComplete.cpp:4825 if (SS.isInvalid()) { -CodeCompletionContext CC(CodeCompletionContext::CCC_Name); +CodeCompletionContext CC(CodeCompletionContext::CCC_Statement); CC.setCXXScopeSpecifier(SS);

[PATCH] D53135: Remove top-level using declaration from header files, as these aliases leak.

This revision was automatically updated to reflect the committed changes. Closed by commit rL344337: Remove top-level using declaration from header files, as these aliases leak. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews

[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

sammccall marked 6 inline comments as done. sammccall added inline comments. Comment at: clangd/index/Background.cpp:98 + } + llvm::sys::path::native(AbsolutePath); + ioeric wrote: > Why is this needed? Removed. I copied it from the related code in JSONCompilat

[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

sammccall updated this revision to Diff 169399. sammccall marked 4 inline comments as done. sammccall added a comment. Address comments, add tests. Some changes (enqueue(), blockUntilIdleForTest()) to support testing. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53032 Files:

[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

sammccall updated this revision to Diff 169400. sammccall added a comment. Oops, forgot one comment. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53032 Files: clangd/CMakeLists.txt clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp clangd/Co

[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

sammccall added inline comments. Comment at: clangd/ClangdLSPServer.h:117 +llvm::Optional CompileCommandsDir, +std::function); ioeric wrote: > please document what the callback is for and how often it's called. Documented at the callsite and in Gl

[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

sammccall added inline comments. Comment at: unittests/clangd/BackgroundIndexTests.cpp:14 + +TEST(BackgroundIndexTest, IndexesOneFile) { + MockFSProvider FS; ioeric wrote: > Also add a test for `enqueueAll` with multiple TUs ? Is it important to call `enqueueAll

[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

sammccall added inline comments. Comment at: unittests/clangd/BackgroundIndexTests.cpp:14 + +TEST(BackgroundIndexTest, IndexesOneFile) { + MockFSProvider FS; ioeric wrote: > sammccall wrote: > > ioeric wrote: > > > Also add a test for `enqueueAll` with multiple

[PATCH] D53202: [python] [tests] Remove cdb lookup failure test

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. We could verify the fuzzy-matched results, but we have pretty good testing on the C++ side, so a smoke test here seems fine. Repository: rC Clang https://reviews.llvm.org/D53202 _

[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

sammccall added inline comments. Comment at: unittests/clangd/BackgroundIndexTests.cpp:14 + +TEST(BackgroundIndexTest, IndexesOneFile) { + MockFSProvider FS; ioeric wrote: > sammccall wrote: > > ioeric wrote: > > > sammccall wrote: > > > > ioeric wrote: > > > >

[PATCH] D53213: [clangd] Send CodeAction responses to textDocument/codeAction (LSP 3.8)

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov. I don't bother mirroring the full capabilities struct, just parse the bits we care about. I'll send a new patch to use this approach els

[PATCH] D53266: [clangd] Simplify client capabilities parsing.

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov. Instead of parsing into structs that mirror LSP, simply parse into a flat struct that contains the info we need. This is an exception to

[PATCH] D53213: [clangd] Send CodeAction responses to textDocument/codeAction (LSP 3.8)

sammccall added inline comments. Comment at: clangd/ClangdLSPServer.cpp:338 + Command Cmd; + if (Action.command && Action.edit) +return llvm::None; kadircet wrote: > What would you think about emitting two commands in this case? First the edit > and then t

[PATCH] D53213: [clangd] Send CodeAction responses to textDocument/codeAction (LSP 3.8)

sammccall updated this revision to Diff 169650. sammccall marked an inline comment as done. sammccall added a comment. update comment Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53213 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/Protocol.cpp clan

[PATCH] D53220: Remove possibility to change compile database path at runtime

sammccall added a comment. Thanks for cleaning this up! Comment at: clangd/ClangdLSPServer.cpp:433 reparseOpenedFiles(); } This isn't needed, the compilation database can only be set during initialization. Comment at: clangd/Clangd

[PATCH] D53192: [clangd] Do not query index for new name completions.

sammccall added a comment. (This looks good, of course the Sema patch needs to land first!) Comment at: clangd/CodeComplete.cpp:643 case CodeCompletionContext::CCC_Recovery: + // TODO: Provide identifier based completions for the following two contexts: + case CodeCompleti

[PATCH] D53273: [clangd] Fix some references missing in dynamic index.

sammccall added a comment. Nice fix, just performance concerns. (I do think this might be significant, but measuring dynamic index time before/after for a big TU might show this to be unimportant) Comment at: clangd/index/SymbolCollector.cpp:348 + if (!shouldCollectSymbol(

[PATCH] D53019: [clangd] dump xrefs information in dexp tool.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/index/dex/dexp/Dexp.cpp:61 + Request.Scopes.emplace_back(); + std::tie(Request.Scopes.back(), Request.Query) = + clang::clangd::splitQuali

[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.

sammccall created this revision. sammccall added reviewers: jkorous, ioeric, hokein. Herald added subscribers: cfe-commits, kadircet, arphaman, MaskRay, ilya-biryukov, mgorny. This paves the way for alternative transports (mac XPC, maybe messagepack?), and also generally improves layering: testin

[PATCH] D53019: [clangd] dump xrefs information in dexp tool.

sammccall added inline comments. Comment at: clangd/index/dex/dexp/Dexp.cpp:185 clang::clangd::LookupRequest Request; -Request.IDs = {*SID}; +Request.IDs.insert(IDs.begin(), IDs.end()); bool FoundSymbol = false; hokein wrote: > sammccall wrote:

[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.

sammccall added a comment. This patch is big and hard to navigate. I tried to keep it contained, but JSONRPCDispatcher is pretty tangled. The main parts are: - `Transport.h` is the key new abstraction that should be understood first - `JSONTransport.cpp` is its standard implementation. The raw

[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

sammccall updated this revision to Diff 169694. sammccall marked an inline comment as done. sammccall added a comment. Address comments, strip out of clangdlspserver for now. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53032 Files: clangd/CMakeLists.txt clangd/ClangdServ

[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

sammccall added inline comments. Comment at: unittests/clangd/BackgroundIndexTests.cpp:14 + +TEST(BackgroundIndexTest, IndexesOneFile) { + MockFSProvider FS; sammccall wrote: > ioeric wrote: > > sammccall wrote: > > > ioeric wrote: > > > > sammccall wrote: > > >

[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE344513: [clangd] Minimal implementation of automatic static index (not enabled). (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53032?vs=169694&id=169695#to

[PATCH] D53288: [clangd] Optionally use dex for the preamble parts of the dynamic index.

sammccall created this revision. sammccall added a reviewer: hokein. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov. Reuse the old -use-dex-index experiment flag for this. To avoid breaking the tests, make Dex deduplicate symbols, addressing an

[PATCH] D53292: [clangd] Add createIndex in dexp

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/index/dex/dexp/Dexp.cpp:256 +std::unique_ptr createIndex(llvm::StringRef Index) { + return loadIndex(Index, /*URISchemes=*/{}, /*UseDex=*/true

[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.

sammccall added a comment. For reference, @jkorous has a WIP in https://reviews.llvm.org/D53290. It's scope is a superset, and I think everything in common is basically the same (they were both based on a common prototype). Jan is out at the moment, so I think it makes sense to move ahead with th

[PATCH] D53220: Remove possibility to change compile database path at runtime

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. I think it'd be a good idea to separate out the on-initialization vs dynamically-changing parameters more - I think they should probably be disjoint in fact. But we can discuss/implemen

[PATCH] D53312: [clangd] Support limiting down traversals from sources in FileDistance.

sammccall added a comment. The slightly weird down-traversals semantics are indeed slightly weird. I'm happy to move forward with this approach though if we document it. I did think of one alternative: - instead of `unsigned Source::MaxDownTraversals`, have `bool Source::AllowDownTraversals` (

[PATCH] D53313: [clangd] Fix threading bugs in (not-yet-used) BackgroundIndex, re-enable test.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, jfb, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. One relatively boring bug: forgot to notify the CV after enqueue. One much more fun bug: the thread member could access instance v

[PATCH] D53288: [clangd] Optionally use dex for the preamble parts of the dynamic index.

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rL344594: [clangd] Optionally use dex for the preamble parts of the dynamic index. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits.

[PATCH] D53313: [clangd] Fix threading bugs in (not-yet-used) BackgroundIndex, re-enable test.

This revision was automatically updated to reflect the committed changes. Closed by commit rL344595: [clangd] Fix threading bugs in (not-yet-used) BackgroundIndex, re-enable test. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://revie

[PATCH] D53317: [clangd] Allow disble down traversals from root.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Works for me! I think the code can be simplified slightly. Comment at: clangd/FileDistance.cpp:135 } + if ((KnownNode == RootHash) && !Opts.AllowDownTraversalFromRo

[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.

sammccall updated this revision to Diff 169807. sammccall added a comment. Add unit test for JSON transport. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53286 Files: clangd/CMakeLists.txt clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/JSONRPCDispatcher.cp

[PATCH] D53131: [clangd] Support scope proximity in code completion.

sammccall added inline comments. Comment at: clangd/AST.cpp:77 + for (const auto *Ctx = &DC; Ctx != nullptr; Ctx = Ctx->getParent()) +if (const auto *NS = dyn_cast(Ctx)) + if (!NS->isAnonymousNamespace() && !NS->isInlineNamespace()) It would be nice to

[PATCH] D53322: [clangd] Collect refs from headers.

sammccall added a comment. This looks reasonable. +80% to index size is a shame, but not disastrous, and we have ways to shave it. Can you check: - how much we're increasing the in-memory size (Dex) - that we're not outputting duplicate refs when preambles overlap between TUs ===

[PATCH] D53322: [clangd] Collect refs from headers.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. (please do check there are no duplicates in the output) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53322 ___ cfe-commi

[PATCH] D53220: Remove possibility to change compile database path at runtime

sammccall accepted this revision. sammccall added a comment. Still LG! Comment at: clangd/Protocol.h:422 +struct ClangdInitializationOptions : public ClangdConfigurationParamsChange { + llvm::Optional compilationDatabasePath; +}; simark wrote: > sammccall wrot

[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.

sammccall marked 9 inline comments as done. sammccall added inline comments. Comment at: clangd/ClangdLSPServer.cpp:156 +void ClangdLSPServer::onExit(ExitParams &Params) { + // XXX handler should return true. +} ioeric wrote: > ? Fixed this comment. I'm not tota

[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.

sammccall updated this revision to Diff 169835. sammccall marked 9 inline comments as done. sammccall added a comment. Addressed review comments. Also inverted the sense of the boolean return from `onNotify` etc, was "should exit" and is now "should continue", which I think is slightly clearer.

[PATCH] D53213: [clangd] Send CodeAction responses to textDocument/codeAction (LSP 3.8)

sammccall updated this revision to Diff 169837. sammccall added a comment. rebase only Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53213 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/Protocol.cpp clangd/Protocol.h test/clangd/fixits-codeaction.t

[PATCH] D53213: [clangd] Send CodeAction responses to textDocument/codeAction (LSP 3.8)

sammccall added inline comments. Comment at: clangd/Protocol.h:390 + /// Flattened from codeAction.codeActionLiteralSupport. + // FIXME: flatten other properties in this way. + bool codeActionLiteralSupport = false; kadircet wrote: > sammccall wrote: > > kadir

[PATCH] D53213: [clangd] Send CodeAction responses to textDocument/codeAction (LSP 3.8)

This revision was automatically updated to reflect the committed changes. Closed by commit rL344617: [clangd] Send CodeAction responses to textDocument/codeAction (LSP 3.8) (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.

[PATCH] D53266: [clangd] Simplify client capabilities parsing.

sammccall updated this revision to Diff 169839. sammccall added a comment. Rebase to include CodeAction literal support Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53266 Files: clangd/ClangdLSPServer.cpp clangd/Protocol.cpp clangd/Protocol.h Index: clangd/Protocol.h =

[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.

This revision was automatically updated to reflect the committed changes. sammccall marked 5 inline comments as done. Closed by commit rL344620: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits

[PATCH] D53266: [clangd] Simplify client capabilities parsing.

This revision was automatically updated to reflect the committed changes. Closed by commit rL344673: [clangd] Simplify client capabilities parsing. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D53266 Files: cla

[PATCH] D53131: [clangd] Support scope proximity in code completion.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Awesome! Just nits. Comment at: clangd/CodeComplete.cpp:1224 std::vector QueryScopes; // Initialized once Sema runs. + // Initialized once QueryScopes is initialize

[PATCH] D53363: [clangd] Encode Line/Column as a 32-bits integer.

sammccall added a comment. (I think your math is off in the description: 20 bits should be 1M lines, not 4M) I think this is a win, as I think truncation will be rare and not terrible. We should document the intentions around truncation though. Incidentally, this means replacing just the String

[PATCH] D53387: [clangd] Lay JSONRPCDispatcher to rest.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, kadircet, jfb, arphaman, jkorous, MaskRay, javed.absar, ilya-biryukov, mgorny. Most of its functionality is moved into ClangdLSPServer. The decoupling between JSONRPCDispatcher, ProtocolCal

[PATCH] D53374: [clangd] Names that are not spelled in source code are reserved.

sammccall added a comment. Idea looks good, I think it needs some renames and index support. Comment at: clangd/AST.h:33 +// `-DName=foo`, the spelling location will be "". +bool SpelledInSourceCode(const Decl *D); + nit: should start with lowercase: `isSpe

[PATCH] D53363: [clangd] Encode Line/Column as a 32-bits integer.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Nice, just nits! In https://reviews.llvm.org/D53363#1267721, @hokein wrote: > Yeah, I have a rough patch for it, using char* will save us ~50MB memory, > which will lead to ~300 MB memo

[PATCH] D53389: [clangd] Clear the semantic of RefSlab::size.

sammccall added a comment. Adding numRefs() and fixing occurrences makes sense. However I think `size()` needs to stay - it's an important part of the "container" concept, and e.g. GTest relies on it. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53389 _

[PATCH] D53374: [clangd] Names that are not spelled in source code are reserved.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/Quality.cpp:181 if (SemaCCResult.Declaration) { +if (!isSpelledInSourceCode(SemaCCResult.Declaration)) + ImplementationDetail = true;

[PATCH] D53391: [clangd] Embed fixes as CodeAction, instead of clangd_fixes. Clean up serialization.

sammccall created this revision. sammccall added reviewers: hokein, arphaman. Herald added subscribers: cfe-commits, kadircet, jkorous, MaskRay, ioeric, ilya-biryukov. CodeAction provides us with a standard way of representing fixes inline, so use it, replacing our existing ad-hoc extension. Aft

[PATCH] D53387: [clangd] Lay JSONRPCDispatcher to rest.

sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clangd/ClangdLSPServer.cpp:148 + if (Trace) +(*Trace)["Reply"] = *Result; + Server.reply(ID, json::Value(std::move(*Result))); ioeric wrote: > d

[PATCH] D53387: [clangd] Lay JSONRPCDispatcher to rest.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE344737: [clangd] Lay JSONRPCDispatcher to rest. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53387?vs=170038&id=170072#toc Repository: rCTE Clang Tools

[PATCH] D53398: [clangd] Enforce rules around "initialize" request, and create ClangdServer lazily.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. LSP is a slightly awkward map to C++ object lifetimes: the initialize request is part of the protocol and provides information that does

[PATCH] D53399: [clangd] Ensure that we reply to each call exactly once. NFC (I think!)

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, kadircet, jfb, arphaman, jkorous, MaskRay, ilya-biryukov. In debug builds, getting this wrong will trigger asserts. In production builds, it will send an error reply if none was sent, and d

[PATCH] D53398: [clangd] Enforce rules around "initialize" request, and create ClangdServer lazily.

This revision was automatically updated to reflect the committed changes. Closed by commit rL344741: [clangd] Enforce rules around "initialize" request, and create ClangdServer… (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews

[PATCH] D53400: [clangd] Remove the overflow log.

sammccall added a comment. After looking at the examples, I'm reassured that we don't really care about tracking precise locations of those references :-) I'd suggest adding the logging just to where the field is *read* in XRefs.cpp (check if it's max-value). That would cover a bunch of the cas

[PATCH] D53400: [clangd] Remove the overflow log.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Some nitty ideas about the log message, but whatever you think is best. Comment at: clangd/XRefs.cpp:43 + if (Line >= SymbolLocation::Position::MaxLine) +log("Get

[PATCH] D53404: [clangd] Set workspace root when initializing ClangdServer, disallow mutation.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Rename instance variable to WorkspaceRoot to match what we call it internally. Add fixme to set it automatically. Don't do it yet, clie

[PATCH] D53406: [clangd] Provide excuses for bad code completions, based on diagnostics. C++ API only.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. In some circumstances we provide bad completions or no completions, because of problems in the code. This can be puzzling and opaque to

<    1   2   3   4   5   6   7   8   9   10   >