[PATCH] D38989: [clang-rename] Rename enum.

ioeric added inline comments. Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:212 + // Ignore the case where there is no prefix qualifer for the enum constant + // expression like `a = Green`. + if (!Expr->hasQualifier()) Why do we ign

[PATCH] D38985: [refactor] Add support for editor commands that connect IDEs/editors to the refactoring actions

ioeric added inline comments. Comment at: include/clang/Tooling/Refactoring/EditorCommandRegistry.def:5 + +REFACTORING_EDITOR_COMMAND(ExtractFunction, "Extract Function") + Isn't `rename` also supported? Comment at: include/clang/Tooling/Refact

[PATCH] D38989: [clang-rename] Rename enum.

ioeric added inline comments. Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:212 + // Ignore the case where there is no prefix qualifer for the enum constant + // expression like `a = Green`. + if (!Expr->hasQualifier()) hokein wrote:

[PATCH] D38989: [clang-rename] Rename enum.

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. Lg Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:212 + // Ignore the case where there is no prefix qualifer for the enum constant + // expression lik

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric updated this revision to Diff 119314. ioeric added a comment. - Fix broken unit tests when they are run in threads in the same process. https://reviews.llvm.org/D34272 Files: include/clang/Tooling/CommonOptionsParser.h include/clang/Tooling/Execution.h include/clang/Tooling/ToolExe

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric updated this revision to Diff 119325. ioeric marked 2 inline comments as done. ioeric added a comment. - Move StandaloneToolExecutor into a separate header; added ExecutionTest.cpp. https://reviews.llvm.org/D34272 Files: include/clang/Tooling/CommonOptionsParser.h include/clang/Tooli

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric updated this revision to Diff 119331. ioeric marked an inline comment as done. ioeric added a comment. - Addressed review comments. https://reviews.llvm.org/D34272 Files: include/clang/Tooling/CommonOptionsParser.h include/clang/Tooling/Execution.h include/clang/Tooling/StandaloneE

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric added a comment. Thanks for the review! Comment at: include/clang/Tooling/CommonOptionsParser.h:116 + bool HasError; + std::string ErrorMessage; std::unique_ptr Compilations; arphaman wrote: > Would it be better to have an `llvm::Error' instead of t

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric updated this revision to Diff 119337. ioeric added a comment. - Minor cleanup in tests. https://reviews.llvm.org/D34272 Files: include/clang/Tooling/CommonOptionsParser.h include/clang/Tooling/Execution.h include/clang/Tooling/StandaloneExecution.h include/clang/Tooling/ToolExecu

[PATCH] D39042: [Tooling] Add a factory method for CommonOptionsParser

ioeric created this revision. This returns error instead of exiting the program in case of error. https://reviews.llvm.org/D39042 Files: include/clang/Tooling/CommonOptionsParser.h lib/Tooling/CommonOptionsParser.cpp Index: lib/Tooling/CommonOptionsParser.cpp ==

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric updated this revision to Diff 119447. ioeric added a comment. - Add a factory method for creating CommonOptionsParser. - Minor cleanup in CommonOptionsParser. - Revert CommonOptionsParser changes. - Merge branch 'option' into arcpatch-D34272 https://reviews.llvm.org/D34272 Files: inclu

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric updated this revision to Diff 119449. ioeric marked 7 inline comments as done. ioeric added a comment. - Address review comments. https://reviews.llvm.org/D34272 Files: include/clang/Tooling/CommonOptionsParser.h include/clang/Tooling/Execution.h include/clang/Tooling/StandaloneExe

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric added inline comments. Comment at: include/clang/Tooling/CommonOptionsParser.h:90 /// - /// This constructor exits program in case of error. + /// If \p ExitOnError is set (default), This constructor exits program in case + /// of error; otherwise, this sets the err

[PATCH] D39043: [clang-rename] Rename alias.

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. Lg https://reviews.llvm.org/D39043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

[PATCH] D39042: [Tooling] Add a factory method for CommonOptionsParser

ioeric updated this revision to Diff 119465. ioeric marked 2 inline comments as done. ioeric added a comment. - Address review comments. https://reviews.llvm.org/D39042 Files: include/clang/Tooling/CommonOptionsParser.h lib/Tooling/CommonOptionsParser.cpp Index: lib/Tooling/CommonOptionsPa

[PATCH] D39042: [Tooling] Add a factory method for CommonOptionsParser

ioeric added inline comments. Comment at: include/clang/Tooling/CommonOptionsParser.h:115 + + static llvm::Error init(int &argc, const char **argv, + llvm::cl::OptionCategory &Category, hokein wrote: > We can get rid of the `static` here

[PATCH] D38982: [refactor] Initial outline of implementation of "extract function" refactoring

ioeric added a comment. Code looks good in general. I see there are a bunch of major features missing; it might make sense to print a warning or document the major missing features in the high-level API. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequ

[PATCH] D38985: [refactor] Add support for editor commands that connect IDEs/editors to the refactoring actions

ioeric added inline comments. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:58 + /// Returns the editor command that's was bound to this rule. + virtual const EditorCommand *getEditorCommand() { return nullptr; } }; arphaman wrote: > io

[PATCH] D39042: [Tooling] Add a factory method for CommonOptionsParser

ioeric updated this revision to Diff 119502. ioeric added a comment. - Move unique_ptr result. https://reviews.llvm.org/D39042 Files: include/clang/Tooling/CommonOptionsParser.h lib/Tooling/CommonOptionsParser.cpp Index: lib/Tooling/CommonOptionsParser.cpp =

[PATCH] D39086: Performance tracing facility for clangd.

ioeric added a comment. Looks good in general. A few nits. Comment at: clangd/JSONRPCDispatcher.cpp:230 - // Finally, execute the action for this JSON message. - if (!Dispatcher.call(JSONRef, Out)) -Out.log("JSON dispatch failed!\n"); + { +// Fi

[PATCH] D39086: Performance tracing facility for clangd.

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D39086 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric updated this revision to Diff 119580. ioeric added a comment. - Remove unused variable. - Narrow supported actions to FrontendActionFactory from ToolAction. https://reviews.llvm.org/D34272 Files: include/clang/Tooling/CommonOptionsParser.h include/clang/Tooling/Execution.h include/

[PATCH] D39042: [Tooling] Add a factory method for CommonOptionsParser

ioeric updated this revision to Diff 119583. ioeric added a comment. - Make the factory method return an expected object instead of unique_ptr. - clang-format code. https://reviews.llvm.org/D39042 Files: include/clang/Tooling/CommonOptionsParser.h lib/Tooling/CommonOptionsParser.cpp Index:

[PATCH] D39120: [rename] Don't overwrite the template argument when renaming a template function.

ioeric added inline comments. Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:227 +SourceLocation EndLoc = Expr->hasExplicitTemplateArgs() +? Expr->getLAngleLoc().getLocWithOffset(-1) +: Expr->getLocE

[PATCH] D39120: [rename] Don't overwrite the template argument when renaming a template function.

ioeric added inline comments. Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:227 +SourceLocation EndLoc = Expr->hasExplicitTemplateArgs() +? Expr->getLAngleLoc().getLocWithOffset(-1) +: Expr->getLocE

[PATCH] D39120: [rename] Don't overwrite the template argument when renaming a template function.

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D39120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D39178: [rename] support renaming class member.

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. Lgtm. Nice! Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:251 + // Ignore implicit initializers. + if (!Initializer->isWritten()) +continue; -

[PATCH] D39042: [Tooling] Add a factory method for CommonOptionsParser

This revision was automatically updated to reflect the committed changes. Closed by commit rL316433: [Tooling] Add a factory method for CommonOptionsParser (authored by ioeric). Repository: rL LLVM https://reviews.llvm.org/D39042 Files: cfe/trunk/include/clang/Tooling/CommonOptionsParser.h

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric updated this revision to Diff 120068. ioeric added a comment. - Experimented and refined the interfaces. o Get rid of `ExecutionConfig` class; pass actions into executors directly. o Add ArgumentAdjuster to ExecutionContext. o Make command-line based executor selection more explicit by r

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric marked an inline comment as done. ioeric added a comment. PTAL I experimented with our internal tools (incl. mapreduce-based execution) and modified the interfaces a bit: o Get rid of `ExecutionConfig` class; pass actions into executors directly. o Moved `ArgumentAdjuster`s that are commo

[PATCH] D39241: [clang-rename] Fix and enable the failing TemplatedClassFunction test.

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lg Comment at: test/clang-rename/TemplatedClassFunction.cpp:8 int main(int argc, char **argv) { A a; + a.foo(); /* Test 2 */ // CHECK: a.bar(); /* Test 2 */

[PATCH] D38982: [refactor] Initial outline of implementation of "extract function" refactoring

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. Lgtm Comment at: include/clang/Basic/DiagnosticRefactoringKinds.td:24 + "not overlap with the AST nodes of interest">; + +def err_refactor_code_outside_of_function : Error<"

[PATCH] D39055: [refactor] Add an editor client that is used in clangd

ioeric added inline comments. Comment at: include/clang/Tooling/Refactoring/EditorClient.h:46 + /// error otherwise. + Expected performRefactoring(ASTContext &Context, + StringRef CommandName, I think it's worth think

[PATCH] D39178: [rename] support renaming class member.

ioeric accepted this revision. ioeric added a comment. Still lgtm. https://reviews.llvm.org/D39178 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric updated this revision to Diff 120232. ioeric added a comment. - Get rid of the unused 'createExecutorByName' interface. https://reviews.llvm.org/D34272 Files: include/clang/Tooling/CommonOptionsParser.h include/clang/Tooling/Execution.h include/clang/Tooling/StandaloneExecution.h

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric updated this revision to Diff 120250. ioeric added a comment. - Only expose result reporting interface from ExecutionContext so that callbacks don't have access to results. https://reviews.llvm.org/D34272 Files: include/clang/Tooling/CommonOptionsParser.h include/clang/Tooling/Execu

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric updated this revision to Diff 120254. ioeric added a comment. - Minor comment update. https://reviews.llvm.org/D34272 Files: include/clang/Tooling/CommonOptionsParser.h include/clang/Tooling/Execution.h include/clang/Tooling/StandaloneExecution.h include/clang/Tooling/ToolExecuto

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

ioeric added inline comments. Comment at: include/clang/Tooling/Execution.h:51-53 + virtual std::vector> AllKVResults() = 0; + virtual void forEachResult( + llvm::function_ref Callback) = 0; ioeric wrote: > klimek wrote: > > Why do we need to get the resul

[PATCH] D38985: [refactor] Add support for editor commands that connect IDEs/editors to the refactoring actions

ioeric added inline comments. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:60 + /// associated with this rule. + virtual Optional> getEditorCommandInfo() { +return None; I think `getEditorCommandInfo` might be a wrong name here. IM

[PATCH] D39290: [rename] Use SymbolOccurrence in RenameLocFinder.

ioeric added a comment. Could you elaborate on the intention of this change? Is the intention of having more information in `SymbolOccurrence` to benefit users of the rename library or to simplify the internal implementation? Comment at: include/clang/Tooling/Refactoring/Rena

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

ioeric created this revision. ioeric added reviewers: ilya-biryukov, simark. Herald added a subscriber: cfe-commits. This partially rolls back the change in https://reviews.llvm.org/D48903: https://github.com/llvm-mirror/clang/commit/89aa7f45a1f728144935289d4ce69d8522999de0#diff-0025af005307891b54

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

ioeric updated this revision to Diff 162166. ioeric added a comment. - Add comment Repository: rC Clang https://reviews.llvm.org/D51159 Files: lib/Basic/FileManager.cpp Index: lib/Basic/FileManager.cpp === --- lib/Basic/File

[PATCH] D51155: [clangd] Allow to merge symbols on-the-fly in global-symbol-builder

ioeric added inline comments. Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:61 +/// rely on MR use-case to work properly. +llvm::cl::init(false)); + ilya-biryukov wrote: > ioeric wrote: > > ilya-biryukov wrote: > > > ioeric wrote: >

[PATCH] D51164: [Tooling] Add a isSingleProcess() helper to ToolExecutor

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm Repository: rC Clang https://reviews.llvm.org/D51164 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bi

[PATCH] D51155: [clangd] Allow to merge symbols on-the-fly in global-symbol-builder

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:60 +"MapReduce."), +llvm::cl::init(true)); + Maybe make this hidden?

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

ioeric updated this revision to Diff 162221. ioeric marked 5 inline comments as done. ioeric added a comment. Herald added subscribers: jfb, javed.absar. - Moved most logic into CodeComplete.cc Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50962 Files: clangd/ClangdServer.cp

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

ioeric updated this revision to Diff 16. ioeric added a comment. - minor cleanup Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50962 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/TUScheduler.h clangd/i

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

ioeric updated this revision to Diff 162224. ioeric added a comment. - another minior cleanup Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50962 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/TUScheduler.h

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

ioeric added inline comments. Comment at: clangd/ClangdServer.cpp:159 + } + if (SpecFuzzyReq) { +if (auto Filter = speculateCompletionFilter(Content, Pos)) { ilya-biryukov wrote: > NIT: maybe invert condition to reduce nesting? It would become something lik

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

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

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

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

[PATCH] D51154: [clangd] Log memory usage of DexIndex and MemIndex

ioeric added a comment. Do we plan to expose an API in `ClangdServer` to allow C++ API users to track index memory usages? Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:123 + size_t Bytes = Index.estimateMemoryUsage(); + for (const auto &Scheme : URISchemes) { +

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

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

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

ioeric added inline comments. Herald added a subscriber: kadircet. Comment at: clangd/SourceCode.cpp:209 + llvm::SmallString<128> RealPath; + if (SourceMgr.getFileManager().getVirtualFileSystem()->getRealPath( + Path, RealPath)) { With the recent perfo

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

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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

ioeric updated this revision to Diff 162335. ioeric added a comment. - fix typos.. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50962 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/TUScheduler.h clangd/ind

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

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

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

ioeric updated this revision to Diff 162337. ioeric added a comment. - Compute absolute paths even when file is not opened. Repository: rC Clang https://reviews.llvm.org/D51159 Files: lib/Basic/FileManager.cpp Index: lib/Basic/FileManager.cpp =

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

ioeric added inline comments. Comment at: clangd/index/SymbolCollector.h:74 +// If not null, SymbolCollector will collect symbols. +const CollectSymbolOptions *SymOpts; +// If not null, SymbolCollector will collect symbol occurrences. Use `llvm::Optio

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC340598: [FileManager] Do not call 'real_path' in getFile(). (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D51159?vs=162337&id=162344#toc Repository: rC Clang

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

ioeric updated this revision to Diff 162348. ioeric marked 5 inline comments as done. ioeric added a comment. - Merge remote-tracking branch 'origin/master' into speculate-index - address comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50962 Files: clangd/ClangdServe

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

ioeric updated this revision to Diff 162351. ioeric added a comment. - fix doc Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50962 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/TUScheduler.h clangd/index/I

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

ioeric added inline comments. Comment at: clangd/index/SymbolCollector.cpp:241 -SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {} +class SymbolCollector::CollectOccurrence { +public: I don't see a strong reason for the separation of `Col

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

ioeric updated this revision to Diff 162355. ioeric added a comment. - moved SpecReq into CodeComplete.cpp Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50962 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/TU

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

ioeric updated this revision to Diff 162356. ioeric added a comment. - add doc Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50962 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/TUScheduler.h clangd/index/I

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE340604: [clangd] Speculative code completion index request before Sema is run. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D50962?vs=162356&id=162358#toc Re

[PATCH] D51291: [clangd] *Prototype* Support multiple #include headers in one symbol.

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Currently, a symbol can have only one #include header attached, which might not work well if the symbol can be imported via different #incl

[PATCH] D51291: [clangd] *Prototype* Support multiple #include headers in one symbol.

ioeric added a comment. @sammccall The code still needs cleanup but should be useful for providing high-level feedback, which I would like to get before moving further. Thanks! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51291 _

[PATCH] D51291: [clangd] *Prototype* Support multiple #include headers in one symbol.

ioeric updated this revision to Diff 162650. ioeric added a comment. - minor cleanup Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51291 Files: clangd/CodeComplete.cpp clangd/Quality.cpp clangd/Quality.h clangd/index/Index.cpp clangd/index/Index.h clangd/index/Merg

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

ioeric added inline comments. Comment at: lib/Basic/SourceManager.cpp:1626 +if (FileContentCache->OrigEntry == SourceFile) { + if (Callback(FileID::get(I))) +return true; Should we check `FileID::get(I)` is valid? Comment at: l

[PATCH] D51297: [docs] Create a guide for Vim users on how to setup Clangd

ioeric added a comment. In https://reviews.llvm.org/D51297#1214297, @ilya-biryukov wrote: > Do we want to keep the docs for different editors separate or do we want to > put them all into a single page in case we add more editors? > I would vouch for keeping them on a single page, but that's no

[PATCH] D51349: [clangd] Use buffered llvm::errs() in the clangd binary.

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Unbuffered stream can cause significant (non-deterministic) latency for the logger. Repository: rCTE Clang Tools Extra https://review

[PATCH] D51349: [clangd] Use buffered llvm::errs() in the clangd binary.

This revision was automatically updated to reflect the committed changes. ioeric marked an inline comment as done. Closed by commit rCTE340822: [clangd] Use buffered llvm::errs() in the clangd binary. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D51349?

[PATCH] D51352: [clangd] Switch to Dex by default for the static index

ioeric added inline comments. Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:32 static llvm::cl::opt -UseDex("use-dex-index", I think we should stick to the same option and just flip the default. Introducing yet another option (that is going to

[PATCH] D51352: [clangd] Switch to Dex by default for the static index

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:32 +// FIXME(kbobyrev): This option should be removed as Dex is now the default +// static index. -

[PATCH] D51291: [clangd] *Prototype* Support multiple #include headers in one symbol.

ioeric updated this revision to Diff 162897. ioeric marked 3 inline comments as done. ioeric retitled this revision from "[clangd] Support multiple #include headers in one symbol." to "[clangd] *Prototype* Support multiple #include headers in one symbol.". ioeric edited the summary of this revisi

[PATCH] D51291: [clangd] Support multiple #include headers in one symbol.

ioeric added a comment. Thanks for the review! I reduced the scope of the patch. PTAL Comment at: clangd/CodeComplete.cpp:1396 + if (IndexResult && !IndexResult->IncludeHeaders.empty()) { +for (const auto &P : IndexResult->IncludeHeaders) + AddWithInclude(

[PATCH] D51407: [Tooling] Do not restore working dir in ClangTool

ioeric added inline comments. Comment at: lib/Tooling/Tooling.cpp:419 - } else { -llvm::report_fatal_error("Cannot detect current path: " + - Twine(CWD.getError().message())); Should we still check if `CWD` is correctly set? Rep

[PATCH] D51407: [Tooling] Do not restore working dir in ClangTool

ioeric added inline comments. Comment at: lib/Tooling/Tooling.cpp:419 - } else { -llvm::report_fatal_error("Cannot detect current path: " + - Twine(CWD.getError().message())); ilya-biryukov wrote: > ioeric wrote: > > Should we sti

[PATCH] D51291: [clangd] Support multiple #include headers in one symbol.

ioeric updated this revision to Diff 163074. ioeric added a comment. - minor cleanup Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51291 Files: clangd/CodeComplete.cpp clangd/index/Index.cpp clangd/index/Index.h clangd/index/Merge.cpp clangd/index/SymbolCollector.cpp

[PATCH] D51292: [docs] Update clang-rename documentation

ioeric added inline comments. Comment at: clang-tools-extra/docs/clang-rename.rst:140 + +:program:`clangd `_ uses +:program:`clang-rename` infrastructure to handle renaming requests. Currently, nit: `clangd *shares* the r

[PATCH] D51407: [Tooling] Do not restore working dir in ClangTool

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lg Comment at: lib/Tooling/Tooling.cpp:202 -std::string getAbsolutePath(StringRef File) { +static llvm::Expected getAbsolutePath(vfs::FileSystem &FS, +

[PATCH] D51475: [clangd] Load YAML static index asyncrhonously.

ioeric created this revision. ioeric added reviewers: sammccall, kbobyrev. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov, mgorny. Loading static index can be slow (>10s for LLVM index). This allows the clangd server to be initialized before index is lo

[PATCH] D51475: [clangd] Load YAML static index asynchronously.

ioeric updated this revision to Diff 163276. ioeric added a comment. - revert unintended change Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51475 Files: clangd/tool/ClangdMain.cpp Index: clangd/tool/ClangdMain.cpp ==

[PATCH] D51481: [clangd] Implement proximity path boosting for Dex

ioeric added a comment. Some high-level comments :) Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:45 + void build(std::shared_ptr> Symbols, + llvm::ArrayRef URISchemes); URI schemes are property of `Symbols`. We shouldn't need to pass

[PATCH] D51481: [clangd] Implement proximity path boosting for Dex

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:45 + void build(std::shared_ptr> Symbols, + llvm::ArrayRef URISchemes); sammccall wrote: > ioeric wrote: > > URI schemes are property of `Symbols`. We shouldn't

[PATCH] D51475: [clangd] Load YAML static index asynchronously.

ioeric updated this revision to Diff 163321. ioeric added a comment. - Fix data race. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51475 Files: clangd/tool/ClangdMain.cpp Index: clangd/tool/ClangdMain.cpp ===

[PATCH] D51475: [clangd] Load YAML static index asynchronously.

ioeric added a comment. In https://reviews.llvm.org/D51475#1219133, @ilya-biryukov wrote: > Any reason to not just wait for the index to load? Is this a UX concern or a > problem when experimenting? The index loading can be slow. When using LLVM YAML index, I need to wait for >10s before clan

[PATCH] D51475: [clangd] Load YAML static index asynchronously.

ioeric added a comment. In https://reviews.llvm.org/D51475#1219197, @ilya-biryukov wrote: > In https://reviews.llvm.org/D51475#1219184, @ioeric wrote: > > > The index loading can be slow. When using LLVM YAML index, I need to wait > > for >10s before clangd starts giving me anything useful. We c

[PATCH] D51475: [clangd] Load YAML static index asynchronously.

ioeric added inline comments. Comment at: clangd/tool/ClangdMain.cpp:86 + return nullptr; +Index = AsyncLoad.get(); +return Index.get(); ilya-biryukov wrote: > ioeric wrote: > > ilya-biryukov wrote: > > > ioeric wrote: > > > > ilya-biryukov wrote: >

[PATCH] D51475: [clangd] Load YAML static index asynchronously.

ioeric added a comment. In https://reviews.llvm.org/D51475#1219291, @sammccall wrote: > I don't have a strong opinion on async vs sync - startup time is important > and we shouldn't block simple AST-based functionality on the index, but this > introduces some slightly confusing UX for that spee

[PATCH] D51291: [clangd] Support multiple #include headers in one symbol.

ioeric updated this revision to Diff 163351. ioeric added a comment. - Return all possible #includes in CodeComplete. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51291 Files: clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/index/Index.cpp clangd/index/Index.h

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

ioeric added inline comments. Comment at: clangd/index/FileIndex.h:47 + // The shared_ptr keeps the symbols alive. + std::shared_ptr buildMemIndex(); Maybe avoid hardcoding the index name, so that we could potentially switch to use a different index implemen

[PATCH] D51504: [clangd] Flatten out Symbol::Details. It was ill-conceived, sorry.

ioeric accepted this revision. ioeric added a comment. lg Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51504 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51528: [NFC] Cleanup Dex

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:33 std::vector Result = generateIdentifierTrigrams(Sym.Name); - Result.push_back(Token(Token::Kind::Scope,

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

ioeric added inline comments. Comment at: clangd/index/dex/DexIndex.h:42 + // All symbols must outlive this index. + template DexIndex(Range &&Symbols) { +for (auto &&Sym : Symbols) Why is this constructor needed? I think this could restrict the flexibilit

[PATCH] D51481: [clangd] Implement proximity path boosting for Dex

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:137 + BoostingIterators.push_back( + createBoost(create(It->second), P.second + 10)); + } Could you comment on `P.second + 10` here? It sounds lik

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

ioeric added inline comments. Comment at: clangd/index/MemIndex.h:30 + /// Builds an index from a slab. The shared_ptr manages the slab's lifetime. + static std::shared_ptr build(SymbolSlab Slab); sammccall wrote: > ioeric wrote: > > (It's a bit unfortunate t

[PATCH] D51481: [clangd] Implement proximity path boosting for Dex

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:36 +Result.push_back(PathToken); + } return Result; nit: no need for braces. [llvm-style] Comment at: clang-tools-extra/clangd/index/dex/DexIndex.

[PATCH] D51291: [clangd] Support multiple #include headers in one symbol.

ioeric updated this revision to Diff 163677. ioeric added a comment. - Rebase Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51291 Files: clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/index/Index.cpp clangd/index/Index.h clangd/index/Merge.cpp clangd/index/Sy

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