[PATCH] D51924: [clangd] Add unittests for D51917

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/D51924 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D51917: [CodeCompletion] Enable signature help when initializing class/struct/union members.

ilya-biryukov added inline comments. Comment at: include/clang/Sema/Sema.h:4571 + /// Tries to get decleration for a member field. + ValueDecl *tryLookupCtorInitMemberDecl(CXXRecordDecl *ClassDecl, s/decleration/declaration. Maybe even remove the comment? th

[PATCH] D51214: [clangd] Add options to enable/disable fixits and function argument snippets.

ilya-biryukov added a comment. +1 to adding an option to drop arguments from snippets and removing the option for the fixes. Comment at: clangd/tool/ClangdMain.cpp:197 +static llvm::cl::opt IncludeFixIts( +"include-fixits", sammccall wrote: > ilya-biryuk

[PATCH] D51747: [clangd] Implement deprecation diagnostics with lower severity.

ilya-biryukov added a comment. In https://reviews.llvm.org/D51747#1229066, @sammccall wrote: > A few thoughts here: > > - does CDB describe user or project preferences? unclear. Agree, it's a mix, defaults are from the project but users can add extra flags. > - "show this warning for code I bu

[PATCH] D51214: [clangd] Add options to enable/disable fixits and function argument snippets.

ilya-biryukov added inline comments. Comment at: clangd/tool/ClangdMain.cpp:205 +static llvm::cl::opt EnableFunctionArgSnippets( +"enable-function-arg-snippets", +llvm::cl::desc("Gives snippets for function arguments, when disabled only " sammccall wrote:

[PATCH] D51917: [CodeCompletion] Enable signature help when initializing class/struct/union members.

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

[PATCH] D51628: [clangd] Implement a Proof-of-Concept tool for symbol index exploration

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/dex/dexp/CMakeLists.txt:10 + Dexp.cpp + ) + Should we indent closing parens to the opening one or keep at the start of the line? Let's pick one style and be consistent (the best op

[PATCH] D51628: [clangd] Implement a Proof-of-Concept tool for symbol index exploration

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:55 + +// llvm::formatv string pattern for pretty-printing symbols. +void fuzzyFind(llvm::StringRef UnqualifiedName, const SymbolIndex &Index) { Is this a leftover fr

[PATCH] D50171: [python] [tests] Update test_code_completion

ilya-biryukov added a comment. Will take a closer look. Thanks for finding the offending revision. https://reviews.llvm.org/D50171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51987: [clangd] Rename global-symbol-builder to clangd-symbol-builder.

ilya-biryukov created this revision. ilya-biryukov added reviewers: ioeric, sammccall, kadircet. Herald added subscribers: arphaman, jkorous, MaskRay, mgorny. Given that the indexer binary is put directly into ./bin directory when built, 'clangd-' prefix seems to provide better context to the read

[PATCH] D51987: [clangd] Rename global-symbol-builder to clangd-symbol-builder.

ilya-biryukov updated this revision to Diff 165085. ilya-biryukov added a comment. - Rebase, updated the added test Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51987 Files: clangd/CMakeLists.txt clangd/global-symbol-builder/CMakeLists.txt clangd/global-symbol-builder/G

[PATCH] D51987: [clangd] Rename global-symbol-builder to clangd-symbol-builder.

ilya-biryukov added a comment. We've used the new name internally before and now that we're testing this, we need to be consistent. The 'clangd-symbol-builder' looks like a better choice, so I'm pitching changing upstream first. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D5

[PATCH] D51977: [clangd] Clarify and hide -index flag.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. +1 to making this hidden. Users (most of them) shouldn't care about it. Is there any motivation on why we someone would want to disable it currently? Maybe we want to deprecate it

[PATCH] D51971: [clangd] Use JSON format in benchmark requests reader

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:21 +using clang::clangd::loadIndex; +using clang::clangd::SymbolIndex; We don't need the usings, just shorten the name on usage sites (usages are inside nam

[PATCH] D51987: [clangd] Rename global-symbol-builder to clangd-symbol-builder.

ilya-biryukov added a comment. `clangd-indexer` looks nice and short. @ioeric, WDYT? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51987 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/

[PATCH] D51987: [clangd] Rename global-symbol-builder to clangd-symbol-builder.

ilya-biryukov updated this revision to Diff 165092. ilya-biryukov added a comment. - Rename to clangd-indexer Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51987 Files: clangd/CMakeLists.txt clangd/global-symbol-builder/CMakeLists.txt clangd/global-symbol-builder/GlobalS

[PATCH] D51987: [clangd] Rename global-symbol-builder to clangd-symbol-builder.

ilya-biryukov added a comment. > You beat it to me, but I thought we didn't use indexer as it could be > confused with the actual indexing? Sorry. Not a big deal, can revert back if needed. Mostly like indexer since it's shorter. Also not very confusing, since we call the thing it produces an "

[PATCH] D51987: [clangd] Rename global-symbol-builder to clangd-symbol-builder.

ilya-biryukov added a comment. Thanks for the comments everyone! Will land this tomorrow, since I'm buildcop this week anyway. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51987 ___ cfe-commits mailing list cfe-commits@lists.llvm

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

ilya-biryukov added a comment. Wow, this is getting somewhat complicated. Have you considered rerunning clangd whenever someone changes an option like that? Would that be much more complicated on your side? Not opposed to having an option too, just want to be aware of the costs involved on you

[PATCH] D51971: [clangd] Use JSON format in benchmark requests reader

ilya-biryukov added a comment. In https://reviews.llvm.org/D51971#1232064, @kbobyrev wrote: > Apparently, the parsing seems to be wrong; I should fix that first. Oh, yeah, just use `llvm::json::parse` https://reviews.llvm.org/D51971 ___ cfe-commi

[PATCH] D51989: [clangd] dexp tool uses llvm::cl to parse its flags.

ilya-biryukov added a comment. A small drive-by comment. PS it'd be cool to have an interface to cl that does not rely on global state... Comment at: clangd/index/dex/dexp/Dexp.cpp:163 + +struct { + const char *Name; Maybe use a named struct? C++ is powerful,

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

ilya-biryukov added a comment. If this setting exposed directly the users in Theia or is it something that is exposed via a custom UI (e.g. choosing named build configs or something similar)? In https://reviews.llvm.org/D51725#1232171, @simark wrote: > I was assuming that restarting clangd mig

[PATCH] D51971: [clangd] Use JSON format in benchmark requests reader

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:45 + if (!JSONArray) { +llvm::errs() << "Couldn't parse request.\n"; + } Return from function after error? Comment at: clang-tools-ext

[PATCH] D51996: [clangd] Simplify cancellation public API

ilya-biryukov added a comment. > but the thing you're spending the CPU on is checking for cancellation... Unless checking for cancellation is really cheap (which is doable, I think). We should probably hit some of those in practice before doing something, though. Comment at: c

[PATCH] D51996: [clangd] Simplify cancellation public API

ilya-biryukov added a comment. Will let Kadir finish the review, consider lgtm'ed from me ;-) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51996 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

[PATCH] D52016: [clangd] Don't create child AND and OR iterators with one posting list

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:148 + if (TrigramIterators.size() > 1) TopLevelChildren.push_back(createAnd(move(TrigramIterators))); + else if (TrigramIterators.size() == 1) Maybe special-case a

[PATCH] D51971: [clangd] Use JSON format in benchmark requests reader

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:45 + if (!JSONArray) { +llvm::errs() << "Couldn't parse request.\n"; + } kbobyrev wrote: > ilya-biryukov wrote: > > Return from function after error? > I

[PATCH] D52016: [clangd] Don't create child AND and OR iterators with one posting list

ilya-biryukov added a comment. Great improvement for such a simple change! https://reviews.llvm.org/D52016 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52016: [clangd] Don't create child AND and OR iterators with one posting list

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a nit. Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:148 + if (TrigramIterators.size() > 1) TopLevelChildren.push_back(createAnd(move(T

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/docs/clangd.rst:135 + +:program:`VSCode` provides `vscode-clangd +` Maybe mention it is published

[PATCH] D51971: [clangd] Use JSON format in benchmark requests reader

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:47 + if (!JSONArray || !JSONArray->getAsArray()) { +llvm::errs() << "FATAL ERROR: Couldn't parse request.\n"; +exit(1); If there was a json parsing er

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM https://reviews.llvm.org/D51297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51971: [clangd] Use JSON format in benchmark requests reader

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:48 +llvm::errs() << "Error when parsing json requests file: " + << llvm::toString(JSONArray.takeError()); +exit(1); We should only call

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

ilya-biryukov added a comment. In https://reviews.llvm.org/D51725#1233254, @simark wrote: > I am not sure I understand correctly your last point. Of course, when > restarting clangd, we need to send again a `didOpen` for each file the user > has currently open in the editor. But that should b

[PATCH] D51971: [clangd] Use JSON format in benchmark requests reader

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:52 + if (!JSONArray->getAsArray()) { +llvm::errs() << "Error when parsing JSON arra

[PATCH] D51747: [clangd] Show deprecation diagnostics

ilya-biryukov added a comment. In https://reviews.llvm.org/D51747#1233499, @kadircet wrote: > Np, not planning to land it before we figure out the issue with vim can't > handling lots of diagnostics anyway. Just wondering: what's the problem with Vim? Slowness? Bad UI for diagnostics when the

[PATCH] D52071: [clangd] Don't override the preamble while completing inside it, it doesn't work.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Neat, I was unaware that single-file mode completions actually work nicely in that case. LGTM! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52071 _

[PATCH] D52076: [CodeComplete] Add completions for filenames in #include directives.

ilya-biryukov added a comment. Amazing, can't wait for it to land! Comment at: lib/Lex/Lexer.cpp:1931 + StringRef PartialPath(AfterQuote, CurPtr - 1 - AfterQuote); + auto Slash = PartialPath.rfind('/'); + StringRef Dir = It's also val

[PATCH] D52083: [clangd] Store OR iterator children in heap

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:128 +// Return LHS > RHS. +auto Compare = [](const std::unique_ptr &LHS, NIT: use triple-slash comments. NIT: LHS > RHS seems to be exactly what's defined by this f

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

ilya-biryukov added a comment. ~1% increase in memory usage seems totally fine. Actually surprised it's that small. Overall LG, just a single comment about extra copying. Thanks for the change, looks like a great improvement! Comment at: clangd/index/FileIndex.cpp:84 +Mer

[PATCH] D52083: [clangd] Store OR iterator children in heap

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:128 +// Return LHS > RHS. +auto Compare = [](const std::unique_ptr &LHS, ilya-biryukov wrote: > NIT: use triple-slash comments. > NIT: LHS > RHS seems to be exactly

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

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

[PATCH] D49253: [clangd] Fix category in clangd-vscode's package.json

ilya-biryukov accepted this revision. ilya-biryukov added a reviewer: ilya-biryukov. ilya-biryukov added a comment. Herald added a subscriber: arphaman. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49253 ___ cfe-commits maili

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

ilya-biryukov added a comment. Herald added a subscriber: arphaman. Thanks for putting up this change! It can be really annoying that clangd does not pick up the compile commands that got updated. A few things of the top of my head on why we might want to punt on using the LSP watches: - File

[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.

ilya-biryukov added a subscriber: simark. ilya-biryukov added a comment. Herald added a subscriber: arphaman. A recent change (https://reviews.llvm.org/D49267) is another indication that caching might be doing more wrong than good. I assume the caching does not give us much performance-wise, we

[PATCH] D49010: YAML output for index-while-building

ilya-biryukov added a comment. JSON is a subset of YAML, do we really need both outputs? I've noticed the schema for the output is actually different between YAML and JSON, wouldn't that be confusing? > Should be compatible with the current index format accepted by clangd Do we really want th

[PATCH] D49523: [clangd] Add support for per-file override compilation command

ilya-biryukov added a comment. The extensions itself seems like a reasonable way to provide compile commands for the individual files. In case didChangeConfiguration does not work for you for some reason, happy to take a look at this change too. One drawback of using didChangeConfiguration for c

[PATCH] D49657: [clangd] Make SymbolLocation => bool conversion explicitly.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Scary Definitely LGTM! Comment at: clangd/index/Index.h:45 - operator bool() const { return !FileURI.empty(); } + explicit operator bool() const { retur

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

ilya-biryukov added a subscriber: sammccall. ilya-biryukov added a comment. @sammccall pointed out that I've been looking at a different layer of caching. Clangd does per-directory (to avoid reloading compilation database multiple times) and per-file (to avoid calling into compilation database mu

[PATCH] D49667: [clangd] Tune down quality score for class constructors so that it's ranked after class types.

ilya-biryukov added a comment. Just a drive-by comment. Comment at: clangd/Quality.cpp:211 + case Constructor: +Score *= 1.0f; // Rank class constructors after class types. +break; NIT: This does not seem to change the score, right? Maybe just the assi

[PATCH] D49719: [ClangFormat] Editor integrations inherit default style from clang-format binary

ilya-biryukov added inline comments. Comment at: tools/clang-format/clang-format-sublime.py:35 regions = [] command = [binary, '-style', style] +if style: Should we remove `'-style ', style` elements from the list here? Repository: rC Clang htt

[PATCH] D49667: [clangd] Tune down quality score for class constructors so that it's ranked after class types.

ilya-biryukov added inline comments. Comment at: clangd/Quality.cpp:211 + case Constructor: +Score *= 1.0f; // Rank class constructors after class types. +break; ioeric wrote: > ilya-biryukov wrote: > > NIT: This does not seem to change the score, right?

[PATCH] D49719: [ClangFormat] Editor integrations inherit default style from clang-format binary

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/D49719 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

ilya-biryukov added a comment. In https://reviews.llvm.org/D49267#1171932, @simark wrote: > I guess you mean language client here, not language server. In our case we > already have a client capable of file watching, so it's convenient for us to > do that (plus, this is what the LSP specificat

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

ilya-biryukov added a comment. In https://reviews.llvm.org/D49267#1173291, @simark wrote: > Ok, I agree that having clangd watch files itself could be necessary at some > point (when the client does not support it), but it would have to be > configurable. In our case, we have efficient enough

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

ilya-biryukov added a comment. > if/when we have a native implementation, supporting multiple mechanisms with > different layering requirements to get at most a 2x win in watcher resource > usage seems like a dubious way to spend our complexity budget. I guess the only think that might force us

[PATCH] D49523: [clangd] Add support for per-file override compilation command

ilya-biryukov added a comment. In https://reviews.llvm.org/D49523#1174086, @arphaman wrote: > We actually need both mechanisms. I posted the didChangeConfiguration > extension to https://reviews.llvm.org/D49758. Why do we need both? Can we have only the one from https://reviews.llvm.org/D4975

[PATCH] D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request

ilya-biryukov added a comment. The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with `compile_commands.json` and other CDB plugins. Do you plan to use the overridden commands in conjunction with CDB plugins or do you

[PATCH] D49724: [VFS] Cleanups to VFS interfaces.

ilya-biryukov added a comment. For the record: this got reverted in https://reviews.llvm.org/rL337850 Repository: rL LLVM https://reviews.llvm.org/D49724 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailma

[PATCH] D49780: [clangd] Use a sigmoid style function for #usages boost in symbol quality.

ilya-biryukov added a comment. Overall LG. I'm sure it's an improvement overall, just wanted to get some clarifying comments and references, if that's possible. NIT: a typo in the commit message s/logrithm/logarithm Comment at: clangd/Quality.cpp:200 +// f = 12.0 +/

[PATCH] D49783: [clangd] Do not rebuild AST if inputs have not changed

ilya-biryukov created this revision. ilya-biryukov added a reviewer: ioeric. Herald added subscribers: jfb, arphaman, jkorous, MaskRay, javed.absar. If the contents are the same, the update most likely comes from the fact that compile commands were invalidated. In that case we want to avoid rebuil

[PATCH] D49780: [clangd] Use a sigmoid style function for #usages boost in symbol quality.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clangd/Quality.cpp:194 + if (References >= 10) { +// Use a sigmoid style boosting function, which flats out better for large +// numbe

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

ilya-biryukov added a reviewer: ilya-biryukov. ilya-biryukov added a comment. Comment at: clangd/ClangdLSPServer.cpp:430 CDB.clear(); - -reparseOpenedFiles(); +compileCommandsChangePost(CCChangeData); } ilya-biryukov wrote: > simark wrote: > >

[PATCH] D49783: [clangd] Do not rebuild AST if inputs have not changed

ilya-biryukov updated this revision to Diff 157269. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Use log instead of vlog - Add parens around && Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49783 Files: clangd/TUScheduler.cpp test/clangd

[PATCH] D49783: [clangd] Do not rebuild AST if inputs have not changed

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:357 + +bool CanReuseAST = InputsAreTheSame && OldPreamble == NewPreamble; { ioeric wrote: > nit: `(OldPreamble == NewPreamble)` > > Do you intend to compare the shared pointer inste

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

ilya-biryukov added a comment. Not strictly opposed to this change, but should any reason why the clients can't guarantee they'll send didChangeConfiguration right after clangd is initialized? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49833

[PATCH] D49783: [clangd] Do not rebuild AST if inputs have not changed

ilya-biryukov updated this revision to Diff 157434. ilya-biryukov added a comment. - Move OldPreamble.reset() out of the lock, add a comment Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49783 Files: clangd/TUScheduler.cpp test/clangd/extra-flags.test unittests/clangd/TU

[PATCH] D49783: [clangd] Do not rebuild AST if inputs have not changed

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:360 std::lock_guard Lock(Mutex); + OldPreamble.reset(); if (NewPreamble) ioeric wrote: > ilya-biryukov wrote: > > ioeric wrote: > > > Why reset? > > We don't need the old p

[PATCH] D49783: [clangd] Do not rebuild AST if inputs have not changed

ilya-biryukov added a comment. In https://reviews.llvm.org/D49783#1175688, @simark wrote: > Thanks, that's simple and efficient. I'll update > https://reviews.llvm.org/D49267 (to call `reparseOpenFiles` once again) once > this is merged. LG, thanks! Repository: rL LLVM https://reviews.l

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

ilya-biryukov added a comment. Many thanks! Great cleanup. Just a few nits and we're good to go Comment at: lib/Basic/VirtualFileSystem.cpp:475 InMemoryNodeKind Kind; + Status Stat; + NIT: maybe keep the order of members the same to keep the patch more focu

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

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/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

[PATCH] D49862: [clang-tidy] Fix a crash in fuchsia-multiple-inheritance

ilya-biryukov created this revision. ilya-biryukov added reviewers: juliehockett, ioeric, hokein, aaron.ballman. Herald added a subscriber: xazax.hun. See the test case for a repro. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49862 Files: clang-tidy/fuchsia/MultipleInherit

[PATCH] D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request

ilya-biryukov added a comment. In https://reviews.llvm.org/D49758#1177747, @arphaman wrote: > In https://reviews.llvm.org/D49758#1174629, @ilya-biryukov wrote: > > > The mode of operation where compile commands come from the client seems > > useful, but I wonder if there's any value in mixing it

[PATCH] D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request

ilya-biryukov added inline comments. Comment at: clangd/Protocol.h:429 + // The changes that happened to the compilation database. + llvm::Optional>> + compilationDatabaseChanges; arphaman wrote: > ilya-biryukov wrote: > > - Maybe add a comment that the ke

[PATCH] D49862: [clang-tidy] Fix a crash in fuchsia-multiple-inheritance

ilya-biryukov updated this revision to Diff 157680. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Move the test into an existing file Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49862 Files: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

[PATCH] D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.h:106 CachingCompilationDb CDB; + InMemoryCompilationDb InMemoryCDB; This code starts to be a little hard to follow. Could we extract it into an external class that encapsulates this l

[PATCH] D49862: [clang-tidy] Fix a crash in fuchsia-multiple-inheritance

ilya-biryukov added a comment. Sorry for sending this in instead of waiting for https://reviews.llvm.org/D49158 to land, I hadn't noticed it earlier Repository: rL LLVM https://reviews.llvm.org/D49862 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D49991: [clangd] Do not build AST if no diagnostics were requested

ilya-biryukov created this revision. ilya-biryukov added a reviewer: ioeric. Herald added subscribers: arphaman, jkorous, MaskRay, javed.absar. It can be washed out from the cache before first access anyway, so building it can just be a waste of time. Repository: rCTE Clang Tools Extra https:

[PATCH] D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request

ilya-biryukov added a comment. Just a few nits left. Comment at: clangd/ClangdLSPServer.cpp:422 +std::move(Entry.second.workingDirectory), +llvm::sys::path::filename(File), +std::move(Entry.second.compilati

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

ilya-biryukov added a comment. In https://reviews.llvm.org/D49833#1181651, @malaperle wrote: > Was there any objection to this patch? It would have been nice to have this > before the branching. Sorry for the delay, somehow missed this update in my inbox. In https://reviews.llvm.org/D49833#11

[PATCH] D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused

ilya-biryukov created this revision. ilya-biryukov added a reviewer: ioeric. Herald added subscribers: jfb, arphaman, jkorous, MaskRay, javed.absar. After r338256, clangd stopped reporting diagnostics if WantDiags::No request is followed by a WantDiags::Yes request but the AST can be reused. Rep

[PATCH] D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused

ilya-biryukov updated this revision to Diff 158201. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Added a comment Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50045 Files: clangd/TUScheduler.cpp unittests/clangd/TUSchedulerTests.cpp Ind

[PATCH] D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:379 + +if (DiagsWereReported) { // Take a shortcut and don't build the AST if neither the inputs nor the ioeric wrote: > The implicit condition here is that we can reuse AST and di

[PATCH] D49794: [libclang] Allow skipping warnings from all included files

ilya-biryukov added inline comments. Comment at: include/clang/Driver/Options.td:1745 HelpText<"remap file source paths in debug info">; +def fsuppress_non_errors_from_included_files : Flag<["-"], "fsuppress-non-errors-from-included-files">, + Group, Flags<[CC1Option]>; --

[PATCH] D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused

ilya-biryukov updated this revision to Diff 158216. ilya-biryukov added a comment. - Moved early return into the CanReuseAST branch Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50045 Files: clangd/TUScheduler.cpp unittests/clangd/TUSchedulerTests.cpp Index: unittests/cla

[PATCH] D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:379 + +if (DiagsWereReported) { // Take a shortcut and don't build the AST if neither the inputs nor the ilya-biryukov wrote: > ioer

[PATCH] D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:381 + DiagsWereReported = PrevDiagsWereReported; + if (DiagsWereReported) { +// Take a shortcut and don't report the diagnostics, since they s

[PATCH] D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused

ilya-biryukov updated this revision to Diff 158217. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Check for PrevDiagsWereReported Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50045 Files: clangd/TUScheduler.cpp unittests/clangd/TUSchedul

[PATCH] D49794: [libclang] Allow skipping warnings from all included files

ilya-biryukov added a comment. In https://reviews.llvm.org/D49794#1182078, @yvvan wrote: > I already mentioned in the review inherited by this one that this way covers > also loaded plugins with different consumers. So let's assume that driver > loads clang-tidy and some other plugins and runs

[PATCH] D49991: [clangd] Do not build AST if no diagnostics were requested

ilya-biryukov updated this revision to Diff 158234. ilya-biryukov added a comment. - Rebase onto head Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49991 Files: clangd/TUScheduler.cpp Index: clangd/TUScheduler.cpp ===

[PATCH] D49794: [libclang] Allow skipping warnings from all included files

ilya-biryukov added a comment. In https://reviews.llvm.org/D49794#1182220, @yvvan wrote: > And we already saw, that -isystem is not the best choice for that. Are you referring to the file-locking on Windows? Any other reasons why the -isystem trick might be non-ideal? > And by the way - clang

[PATCH] D49794: [libclang] Allow skipping warnings from all included files

ilya-biryukov added a comment. In https://reviews.llvm.org/D49794#1182296, @yvvan wrote: > Clang tidy is not only a standalone tool but also a plugin. It's almost never > used this way (but we do that in Qt Creator to combine it with Clazy) and it > also seems that some of the recent checks are

[PATCH] D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request

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

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a small nit. Thanks for the change! Comment at: clangd/ClangdLSPServer.cpp:419 +DidChangeConfigurationParams &Params) { + ClangdConfigurationPara

[PATCH] D49658: [clangd] Index Interfaces for Xrefs

ilya-biryukov added a comment. Picking it up since @sammccall is OOO. > Implementing all index operations in the patch would be end up with a large > patch, I intend to split the patch as small as possible. Let's scope this > patch to interface only. LG as interface-only patch. just a few nits

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

ilya-biryukov added subscribers: simark, malaperle. ilya-biryukov added a comment. Maybe we could simply always capitalize the messages? Any cons to capitalizing error messages? @simark, @malaperle, @ioeric, @hokein, WDYT? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50154

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

ilya-biryukov added a comment. Seems we have consensus here. Let's remove the option and just always capitalize the messages. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50154 ___ cfe-commits mailing list cfe-commits@lists.llvm.o

[PATCH] D50189: Fully qualify the renamed symbol if the shortened name is ambiguous.

ilya-biryukov added inline comments. Comment at: lib/Tooling/Core/Lookup.cpp:137 + StringRef TrimmedQName = QName.substr(2); + for (auto I = UseNamespaces.begin(), E = UseNamespaces.end(); I != E; ++I) { +const NamespaceDecl *NS = *I; maybe use range-based-

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

ilya-biryukov added a comment. Thanks for the change! Overall LG, mostly NITs about the tests. Comment at: clangd/CodeComplete.cpp:959 bool Incomplete = false; // Would more be available with a higher limit? - llvm::Optional Filter; // Initialized once Sema runs. - s

[PATCH] D50189: Fully qualify the renamed symbol if the shortened name is ambiguous.

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/D50189 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

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