[PATCH] D50193: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa.

ilya-biryukov added a comment. Thanks! The only major comment I have is about the `FixItsScore`, others are mostly NITs. Comment at: clangd/Quality.cpp:298 +FixItCount += FixIt.CodeToInsert.size(); + } } NIT: remove braces around single-statement loop bo

[PATCH] D50193: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, with a few NITs. Thanks for the change! Comment at: clangd/Quality.h:81 + bool NeedsFixIts = + false; // Whether fixits needs to be applied for that

[PATCH] D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested)

ilya-biryukov added a comment. +1 Sam's suggestion of configuring this during initial LSP handshake, rather than as a command-line flag. We could put it into `ClientCapabilities` or into `initializationOptions`. The latter has type 'any' in LSP, so it seems to be most in-line with the protocol

[PATCH] D50154: [clangd] capitalize diagnostic messages

ilya-biryukov added a comment. In https://reviews.llvm.org/D50154#1191002, @dblaikie wrote: > What's the motivation for clangd to differ from clang here? The presentation of diagnostics to the users is different between clang and clangd: - clang diagnostics are always prefixed with the file,

[PATCH] D50443: [clang] Store code completion token range in preprocessor.This change is to support a new fature in clangd, tests will be send toclang-tools-extra with that change.

ilya-biryukov added a comment. Maybe split the commit message into multiple lines? I suggest we wait for clangd changes to be reviewed and land them together, so that we have the tests and usages of this code. Other than that LG Comment at: include/clang/Lex/Preprocessor.h:11

[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

ilya-biryukov added a comment. Short summary of the offline discussion: I suggest adding this parameter into the corresponding requests of the index (FuzzyFindRequest, FindDefinitionsRequest) instead of stashing it in the context. Context has all the same problems as the global variables, so we

[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:162 +WithContextValue WithFileName(kActiveFile, File); // FIXME(ibiryukov): even if Preamble is non-null, we may want to check ioeric wrote: > ilya-biryukov wrote: > > If we want

[PATCH] D50455: Continue emitting diagnostics after a fatal error

ilya-biryukov added a comment. The new behavior looks reasonable for interactive tools like clangd, but I'm a bit worried that clang may emit too many irrelevant errors, because it may not cope nicely with those fatal errors, i.e. wasn't tested that thoroughly in that mode. Nevertheless, I thin

[PATCH] D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested)

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:441 const clangd::CodeCompleteOptions &CCOpts, + const ClangdDiagnosticOptions &DiagOpts, llvm::Optional

[PATCH] D50446: [clangd] Record the file being processed in a TUScheduler thread in context.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Comment at: clangd/TUScheduler.h:145 }; + } // namespace clangd NIT: accidental whitespace change? Repository: rCTE Clang T

[PATCH] D50449: [clangd] Support textEdit in addition to insertText.

ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:289 } + std::stable_sort(Completion.FixIts.begin(), Completion.FixIts.end(), + [](const TextEdit &X, const TextEdit &Y) { We shouldn't have duplicate/overla

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

ilya-biryukov added a comment. I have left a few comments, but wanted to start with a higher-level design consideration. I believe we should probably expose the cancellations in the ClangdServer API directly. The reasons for that are: 1. We have an internal client that would also benefit from

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

ilya-biryukov added a comment. Only a few NITs from my side. Excited for this fix to get in, have been seeing duplicate in other cases too :-) Comment at: clangd/SourceCode.h:69 +llvm::Optional getRealPath(const FileEntry *F, +const So

[PATCH] D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested)

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a small NIT. Comment at: clangd/ClangdLSPServer.cpp:507 +} +LSPDiag["clangd.fixes"] = std::move(ClangdFixes); + } ---

[PATCH] D50555: [clangd] Introduce scoring mechanism for SignatureInformations.

ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:715 unsigned NumCandidates) override { +TopN Top( +std::numeric_limits::max()); Maybe use `vector`, followed by `std::sort` at the end? Or is t

[PATCH] D50449: [clangd] Support textEdit in addition to insertText.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Thanks for the change! Could we add an option to clangd to switch it on? (VSCode does not work, but our hacked-up ycm integration seems to work, right?)

[PATCH] D50555: [clangd] Introduce scoring mechanism for SignatureInformations.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Thanks! LGTM with a few NITs Comment at: clangd/CodeComplete.cpp:687 +struct ScoredSignatureGreater { + bool operator()(const ScoredSignature &L, const ScoredS

[PATCH] D50443: [clang] Store code completion token range in preprocessor.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, but let's land together with a dependent revision to hove some code that actually tests it. Comment at: include/clang/Lex/Preprocessor.h:313 + /// Ran

[PATCH] D50443: [clang] Store code completion token range in preprocessor.

ilya-biryukov added a comment. NIT: maybe also note the number of the clangd revision in this change's description Repository: rC Clang https://reviews.llvm.org/D50443 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

[PATCH] D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category

ilya-biryukov added inline comments. Comment at: clangd/Diagnostics.h:40 DiagnosticsEngine::Level Severity = DiagnosticsEngine::Note; + unsigned Category; // Since File is only descriptive, we store a separate flag to distinguish Maybe store the string nam

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

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

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D50628 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

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

ilya-biryukov added inline comments. Comment at: clangd/XRefs.cpp:71 +struct DeclInfo { + const Decl *D; NIT: maybe call `Occurence` instead? As this is actually a `Decl` with some extra data, computed based on the expression it originated from. Occurence se

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, kadircet. Herald added subscribers: arphaman, jkorous, MaskRay. To avoid producing very verbose output in substitutions involving typedefs, e.g. T -> std::vector::iterator gets turned into an unreadable mess wh

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

ilya-biryukov updated this revision to Diff 160536. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Use 'auto*' instead of 'auto' Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50645 Files: clangd/CodeComplete.cpp unittests/clangd/CodeComple

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

ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:721 + // would get 'std::basic_string'). + if (auto Func = Candidate.getFunction()) { +if (auto Pattern = Func->getTemplateInstantiationPattern()) hokein wrote: > nit: auto

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

ilya-biryukov added a comment. Sorry for missing this one. The biggest concern that I have is about the thread-safety of the API, it's too easy to misuse from multiple threads at this point. We should define thread-safety guarantees for `TaskHandle` and `CancellationToken` and make sure they'r

[PATCH] D50641: [clangd][test] Fix exit messages in tests

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Many thanks for fixing this. Adding some failure monitoring seems like a nice idea. On the other hand, polluting every test with stderr redirection doesn't look like a nice

[PATCH] D50641: [clangd][test] Fix exit messages in tests

ilya-biryukov added a comment. Haven't noticed Alex's comments, sorry for a duplicate suggestion about exiting with failure error code Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50641 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Let's watch out for possible breakages in any of the clients, though. I've checked VSCode and it seems to be fine with the added fields. Repository: rCTE Clang Tools Ex

[PATCH] D50726: [clangd] Show function documentation in sigHelp

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, kadircet. Herald added subscribers: arphaman, jkorous, MaskRay. Previously, clangd was trying to show documentation for the active parameter instead, which is wrong per LSP specification. Moreover, the code path t

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, kadircet. Herald added subscribers: arphaman, mgrang, jkorous, MaskRay. Sema can only be used for documentation in the current file, other doc comments should be fetched from the index. Repository: rCTE Clang T

[PATCH] D50726: [clangd] Show function documentation in sigHelp

ilya-biryukov added a comment. There's a test for the new behavior in https://reviews.llvm.org/D50726 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50726 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

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

ilya-biryukov updated this revision to Diff 160647. ilya-biryukov added a comment. - run clang-format Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 Files: clangd/ClangdServer.cpp clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/index/Index.cpp clangd/index/

[PATCH] D50785: [clangd][tests] Add exit(EXIT_FAILURE) in case of JSON parsing failure in TestMode

ilya-biryukov added a comment. +1 to having a separate option for that. More generally, wouldn't we want to exit on more kinds errors in the future? JSON parsing errors is something that popped up recently, but the option only for that looks too narrow. We might end up wanting more options like

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

ilya-biryukov added a comment. Thanks, we were previously getting inconsistent ASTs (i.e. using different preambles), depending on whether they were built in `TUScheduler::update` or in `TUScheduler::runWithAST` handlers. Just a few NITs. Comment at: clangd/TUScheduler.cpp:37

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

ilya-biryukov added a comment. Mostly LSP-specific comments and comments about making TaskHandle thread-safe. I'm also starting to wonder if the scope of this patch is too big, we could potentially split it into three separate bits: 1. Generic cancellation API, i.e. `Cancellation.h` and unit-te

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

ilya-biryukov added a comment. In https://reviews.llvm.org/D50627#1198595, @hokein wrote: > I think this patch is in a good scope (for empty preamble). I'd leave it as > it is. The failure of GoTodefinition test is caused by an inconsistent > behavior of using `lastBuiltPreamble`/`NewPreamble`

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

ilya-biryukov added a comment. The last set of comments is for the previous version, might be missing some updates, sorry about that. Will make sure to review the one too! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50502 ___ cf

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

ilya-biryukov updated this revision to Diff 160992. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Log on index request, remove FIXME that was addressed - Remove SymbolID::forDecl, use existing helper instead Repository: rCTE Clang Tools Extra https://reviews

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

ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:742 +llvm::DenseMap FetchedDocs; +if (Index) { + LookupRequest IndexRequest; hokein wrote: > nit: do we want to log anything here? It may be useful for debug. Definitely useful.

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

ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", ioeric wrote: > drive by: I think this

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

ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", ioeric wrote: > hokein wrote: > > ioeri

[PATCH] D50726: [clangd] Show function documentation in sigHelp

ilya-biryukov updated this revision to Diff 161011. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Expose getDeclComment instead of getDocComment Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50726 Files: clangd/CodeComplete.cpp clangd/Cod

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

ilya-biryukov added a comment. In https://reviews.llvm.org/D50385#1193600, @ioeric wrote: > In https://reviews.llvm.org/D50385#1193545, @hokein wrote: > > > In https://reviews.llvm.org/D50385#1191914, @ioeric wrote: > > > > > 2 high-level questions: > > > > > > 1. What's the reason for having a s

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

ilya-biryukov added a comment. Just drive-by comments, the maintainers of the code are in a much better position to give feedback, of course. Nevertheless, my few cents: - Getting rid of a regex in favor of the explicit loop is definitely a good thing. It's incredibly how much time is spent the

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

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: arphaman, jkorous, MaskRay, ioeric, javed.absar. Will be used for updating the dynamic index on updates to the open files. Currently we collect only information coming from the preamble AST. This

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, but note the comment: we don't need `ASSERT_TRUE(bool(Preamble))`. Comment at: unittests/clangd/TUSchedulerTests.cpp:333 + // We expe

[PATCH] D50726: [clangd] Show function documentation in sigHelp

ilya-biryukov updated this revision to Diff 161187. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Turn ifs to ternary ops Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50726 Files: clangd/CodeComplete.cpp clangd/CodeCompletionStrings.cpp

[PATCH] D50726: [clangd] Show function documentation in sigHelp

ilya-biryukov added inline comments. Comment at: clangd/CodeCompletionStrings.cpp:52 // get this declaration, so we don't show documentation in that case. if (Result.Kind != CodeCompletionResult::RK_Declaration) return ""; ioeric wrote: > `RK_Pattern`

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

ilya-biryukov added a comment. In https://reviews.llvm.org/D50847#1202658, @ioeric wrote: > > Dynamic index misses important information from the body of the file, e.g. > > locations of definitions > > XRefs cannot be collected at all, since we can only obtain full > > information for the curr

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric. Herald added subscribers: arphaman, jkorous, MaskRay. It was previously only indexing the preamble decls. The new implementation will index both the preamble and the main AST and report both sets of symbols, prefer

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

ilya-biryukov added a comment. @ioeric, specifically, see https://reviews.llvm.org/D50889 for an updated FileIndex Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50847 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://

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

ilya-biryukov added a comment. This is still WIP, but wanted to share anyway the progress anyway. Need to fix the FIXMEs currently added to the code and add tests. Comment at: clangd/index/FileIndex.cpp:81 if (!Slab) FileToSlabs.erase(Path); else We

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

ilya-biryukov planned changes to this revision. ilya-biryukov added a comment. As discussed offline, we could instead have a two-layer scheme for dynamic index. A top layer will contain main file symbols, second layer will contain preamble symbols. Repository: rCTE Clang Tools Extra https:/

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.h:85 + + /// Enables cursor to be moved around according to completions needs even when + /// snippets are disabled. For example selecting a function with parameters as kadircet wrote: > ioe

[PATCH] D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client

ilya-biryukov added a comment. Why do we want an alternative mode for transferring fix-its and notes? Any reason why our current model is bad? One thing that comes to mind is inconsistency between clang and our model, but I wonder where would it affect the user experience in a bad way? Reposit

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

ilya-biryukov added a comment. Mostly NITs, but please take a look at the `CancelParams` handling problem. I believe there might be a potential bug hiding there :-) Comment at: clangd/Cancellation.h:87 +/// Enables async tasks to check for cancellation signal, contains a read

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

ilya-biryukov added inline comments. Comment at: clangd/XRefs.cpp:665 +std::vector references(ParsedAST &AST, Position Pos, + bool IncludeDeclaration, + const SymbolIndex *Index) { Are we going to su

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

ilya-biryukov updated this revision to Diff 161260. ilya-biryukov added a comment. - Reverted changes to FileIndex, use merged index instead Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50889 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/index/FileIndex.cp

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

ilya-biryukov added a comment. Still missing tests, but otherwise should be good to review. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/m

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

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:406 // We only need to build the AST if diagnostics were requested. if (WantDiags == WantDiagnostics::No) return; hokein wrote: > The AST might not get built if `WantDiags::N

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

ilya-biryukov added a comment. This change LG, but I would not commit it before we have an actual implementation. As soon as we have the `references` function in `ClangdUnit.cpp` implemented, the merge of this change should be trivial. Is there any value in committing empty stubs before an actu

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:80 + if (NormalizedID.front() == '"') +NormalizedID = NormalizedID.substr(1, NormalizedID.size() - 2); + return NormalizedID; This still misses string escaping issues. E.g. `"` will

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

ilya-biryukov added a comment. Having unimplemented stubs in the codebase creates some confusion, but that's my personal opinion. Not terribly opposed to that if others feel it's the right way to go. For experiments, we could simply patch this in locally without committing upstream. With git i

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

ilya-biryukov added inline comments. Comment at: clangd/Cancellation.h:96 +/// checks using it to avoid extra lookups in the Context. +class CancellationToken { +public: ioeric wrote: > As chatted offline, I have questions about the motivation of the > `Cancella

[PATCH] D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client

ilya-biryukov added a comment. Herald added a subscriber: kadircet. Just to make sure I fully understand the use-case: could you elaborate a little more? Do you need to get exactly the same set of notes that clang provides? Our current model seems to follow the clang's model, but it removes some

[PATCH] D50703: [clangd] NFC: Mark Workspace Symbol feature complete in the documentation

ilya-biryukov added a comment. In https://reviews.llvm.org/D50703#1205167, @malaperle wrote: > Nah, I guess other things are partial too, like Go to Definition. Let's > assume they will be completed "soon" ;) https://reviews.llvm.org/D50889 should finally add the symbols from the main files.

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

ilya-biryukov added inline comments. Comment at: clangd/Cancellation.h:96 +/// checks using it to avoid extra lookups in the Context. +class CancellationToken { +public: ioeric wrote: > ilya-biryukov wrote: > > ioeric wrote: > > > As chatted offline, I have quest

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

ilya-biryukov added a comment. 1. Can we put the helper that executes the tasks asynchronously with a stack size into llvm's threading library (i.e. somewhere close to `llvm::execute_on_thread`?) All platform-specific code is already there, we can reuse most of the implementation too. 2. I wond

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

ilya-biryukov updated this revision to Diff 161694. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Move DynamicIndex into ClangdServer.cpp - Rename FileIdx to DynamicIdx - Avoid modifying input args Repository: rCTE Clang Tools Extra https://reviews.llvm.org/

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

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:70 +// FIXME(ibiryukov): this should be a generic helper instead. +class NoopCallbacks : public ParsingCallbacks { public: ioeric wrote: > Maybe provide noop implementations for `ParsingCal

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

ilya-biryukov added a comment. In https://reviews.llvm.org/D50993#1207464, @Dmitry.Kozhevnikov wrote: > Do you think it would be OK to have pthreads-only version there, with > `std::thread` fallback on Windows? I'd like to avoid writing a Windows-only > implementation myself, as it's a bit alie

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

ilya-biryukov updated this revision to Diff 161717. ilya-biryukov added a comment. - Provide noop callbacks implementation by default. - Update docs. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50847 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/TUSchedul

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

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:73 + + void onPreambleAST(PathRef Path, ASTContext &Ctx, + std::shared_ptr PP) override { hokein wrote: > I think `Ctx` should be a `pointer` which gives us a way (passi

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

ilya-biryukov updated this revision to Diff 161718. ilya-biryukov added a comment. - Use noop callbacks from the parent revision Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50889 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/index/FileIndex.cpp clangd/i

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

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. All the issues should be addressed at this point Comment at: clangd/ClangdServer.cpp:70 +// FIXME(ibiryukov): this should be a generic helper instead. +class NoopCallbacks : public ParsingCallbacks {

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

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clangd/index/FileIndex.h:70 + void + update(PathRef Path, ASTContext *AST, std::shared_ptr PP, + llvm::Optional> TopLevelDecls = llvm::None); hokein wrote:

[PATCH] D50970: [clangd] Implement BOOST iterator

ilya-biryukov added a comment. Looking at this one. (to make sure we don't clash with @ioeric) Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:111 + const static unsigned DEFAULT_BOOST_SCORE; + Maybe make it `constexpr` and put the value into the h

[PATCH] D50970: [clangd] Implement BOOST iterator

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:92 + /// function, the parameter is needed to propagate through the tree. + virtual unsigned boost(DocID ID) const = 0; Maybe add a note to the comment on why an `I

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

ilya-biryukov added a comment. LG, just a few last nits Comment at: clangd/Cancellation.cpp:30 + +TaskHandle createTaskHandle() { return std::make_shared(); } + NIT: maybe consider inlining it? Since Task has a public default constructor, I don't think having

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

ilya-biryukov created this revision. ilya-biryukov added reviewers: ioeric, kbobyrev. Herald added subscribers: kadircet, jfb, arphaman, jkorous, MaskRay. Replace them with suffix mappings. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51088 Files: clangd/index/CanonicalIncl

[PATCH] D50455: Continue emitting diagnostics after a fatal error

ilya-biryukov added a comment. Herald added a subscriber: kadircet. Sorry for the delay with this one Comment at: unittests/clangd/ClangdTests.cpp:1002 + + auto MainFileCI = buildCompilerInvocation(PI); + auto AST = Just reuse `PreambleCI`?

[PATCH] D51038: [clang] Make sure codecompletion is called for calls even when inside a token.

ilya-biryukov added a comment. Maybe add tests? Lit tests for code completion (`clang/test/CodeCompletion`) should be a good fit Comment at: include/clang/Parse/Parser.h:2971 + /// signature help working even when it is triggered inside a token. + bool CalledOverloadCompleti

[PATCH] D51077: [clangd] send diagnostic categories only when 'categorySupport' capability was given by the client

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51077 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

ilya-biryukov added a comment. WRT to the configuration argument, using stack size suggested by @arphaman seems like a better option. So let's not add any extra options to clangd. Comment at: clangd/Threading.cpp:76 + + if (::pthread_attr_setstacksize(&Attr, 8 * 1024 * 1024)

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Repository: rC Clang https://reviews.llvm.org/D50967 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:

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

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:73 + + void onPreambleAST(PathRef Path, ASTContext &Ctx, + std::shared_ptr PP) override { hokein wrote: > ilya-biryukov wrote: > > hokein wrote: > > > I think `Ctx` shou

[PATCH] D50970: [clangd] Implement BOOST iterator

ilya-biryukov added a comment. Summarizing the offline discussion with @kbobyrev, @ioeric and @sammccall in two comments. Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:96 + /// shouldn't apply any boosting to the consumed item. + virtual float boost(DocID ID) con

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

ilya-biryukov updated this revision to Diff 161924. ilya-biryukov added a comment. - Rebase onto head Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50889 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/index/FileIndex.cpp clangd/index/FileIndex.h Index: cl

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: clangd/index/CanonicalIncludes.h:44 /// Maps all files matching \p RE to \p CanonicalPath - void addRegexMapping(llvm::StringRef RE, llvm::StringRef CanonicalPath); + void addSuf

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

ilya-biryukov updated this revision to Diff 161932. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - s/addSuffixMapping/addFileSuffixMapping - Document the intention of MaxSuffixComponents Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51088 File

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

ilya-biryukov added inline comments. Comment at: clangd/index/Index.cpp:139 +std::sort(Occurrences.begin(), Occurrences.end(), + [](const SymbolOccurrence &L, const SymbolOccurrence &R) { +return L < R; NIT: remove the lambda? usi

[PATCH] D50970: [clangd] Implement BOOST iterator

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D50970 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

ilya-biryukov updated this revision to Diff 161945. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Use std::distance instead of an explicit loop - Merge break into loop condition Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51088 Files: cla

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

ilya-biryukov added inline comments. Comment at: clangd/index/CanonicalIncludes.cpp:24 + int Components = 0; + for (auto It = llvm::sys::path::begin(Suffix), +End = llvm::sys::path::end(Suffix); ioeric wrote: > Would `int Components = begin(Suffix)

[PATCH] D51038: [clang] Make sure codecompletion is called for calls even when inside a token.

ilya-biryukov added a comment. Sorry, missed the dependent revision adding tests to clangd at first. Having another test in sema would still be great, to make sure we guard against regressions if someone does not have clang-tools-extra checked out. Repository: rC Clang https://reviews.llvm.

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, kbobyrev, sammccall. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. The new mode avoids serializing and deserializing YAML. This results in better performance and less memory usage. Reduce phase is

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

ilya-biryukov added a comment. Collecting the timings to build the index for LLVM now. Will publish when they're ready Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51155 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

  1   2   3   4   5   6   7   8   9   10   >