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

sammccall added a comment. Thanks! @arphaman, any concerns about the change to extension format? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53391 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-

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

sammccall updated this revision to Diff 170153. sammccall marked an inline comment as done. sammccall added a comment. Don't call URIForFile again, rebase. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53391 Files: clangd/ClangdLSPServer.cpp clangd/Diagnostics.cpp clangd

[PATCH] D53433: [clangd] *Prototype* auto-index stores symbols per-file instead of per-TU.

sammccall added inline comments. Comment at: clangd/index/FileIndex.cpp:167 +} +// FIXME: aggregate symbol reference count based on references. +for (const auto &Sym : FileSyms.getValue()->Refs) { This is a good FIXME to carry over to FileSymbols::bui

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

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE344787: [clangd] Set workspace root when initializing ClangdServer, disallow mutation. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53404?vs=170098&id=170

[PATCH] D53439: [clangd] Remove caching of compilation database commands.

sammccall created this revision. sammccall added reviewers: ioeric, ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay. The CDB implementations used in open-source code are fast, and our private slow CDB will soon do the relevant caching itself. Simplifying

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

sammccall added a comment. Nice savings! My initial thought is that it seems weird to only do it for this one string - the readers look different, the serialization code has different logic etc for the different flavors of strings. The struct certainly looks nicer to my eyes. This may be the r

[PATCH] D53456: [Sema] Do not show unused parameter warnings when body is skipped

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Haha, fun! Repository: rC Clang https://reviews.llvm.org/D53456 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llv

[PATCH] D53481: [clangd] Support passing a relative path to -compile-commands-dir

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks for the fix! Comment at: clangd/tool/ClangdMain.cpp:261 } else { -CompileCommandsDirPath = CompileCommandsDir; +// If the compile-commands-dir arg pat

[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.

sammccall added inline comments. Comment at: clangd/index/Background.h:68 + FileSymbols IndexedSymbols; + FileDigests IndexedFileDigests; // Keyed by file URIs. Keying this with file URIs seems suspicious to me - why this change? It seems to be motivated by

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

sammccall updated this revision to Diff 170593. sammccall added a comment. Make ReplyOnce move-only. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53399 Files: clangd/ClangdLSPServer.cpp Index: clangd/ClangdLSPServer.cpp =

[PATCH] D53561: [clang] Fix a null pointer dereference.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Nice! Can you construct an example and add a simple lit test? https://reviews.llvm.org/rL344133 is a recent similar change. Repository: rC Clang https://reviews.llvm.org/D53561 __

[PATCH] D53527: Fix range length comparison in DraftStore::UpdateDraft when Unicode characters are removed from the document

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks for the fix! Just comment nits. Comment at: clangd/DraftStore.cpp:85 + +// The difference between EndIndex and StartIndex gives the range length in +// b

[PATCH] D53481: [clangd] Support passing a relative path to -compile-commands-dir

sammccall added a comment. It looks like these are your first couple of LLVM patches. So, welcome :-) And do you want me to commit them for you? Typically your first few patches get landed by someone else, and then you ask for commit access: https://llvm.org/docs/DeveloperPolicy.html#obtaining-

[PATCH] D53439: [clangd] Remove caching of compilation database commands.

sammccall added a comment. Ping - this seems straightforward, and blocks further cleanups. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53439 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/ma

[PATCH] D53527: Fix range length comparison in DraftStore::UpdateDraft when Unicode characters are removed from the document

This revision was automatically updated to reflect the committed changes. Closed by commit rL345020: Fix range length comparison in DraftStore::UpdateDraft when Unicode characters… (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https:/

[PATCH] D53481: [clangd] Support passing a relative path to -compile-commands-dir

This revision was automatically updated to reflect the committed changes. Closed by commit rL345022: [clangd] Support passing a relative path to -compile-commands-dir (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.o

[PATCH] D53439: [clangd] Remove caching of compilation database commands.

This revision was automatically updated to reflect the committed changes. Closed by commit rL345024: [clangd] Remove caching of compilation database commands. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D5343

[PATCH] D53571: [clangd] Don't show base class versions of members as completions.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. These are available via qualifiers, but signal to noise level is low. Keep required quailifier machinery around though, for cross-ns com

[PATCH] D53572: [clangd] Lazily create CDB, remove setCompileCommandsDir.

sammccall created this revision. sammccall added a reviewer: hokein. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov. The only way to actually set the directory is at initialize time, so now CDB is lazy we can pass it to the constructor. Reposi

[PATCH] D53572: [clangd] Lazily create CDB, remove setCompileCommandsDir.

This revision was automatically updated to reflect the committed changes. Closed by commit rL345031: [clangd] Lazily create CDB, remove setCompileCommandsDir. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D53572 F

[PATCH] D53191: [clang] Introduce new completion context types

sammccall added inline comments. Comment at: include/clang/Sema/CodeCompleteConsumer.h:277 + +/// Code completion occurred where an existing name is expected. +CCC_ExistingName, It's not obvious what "name" means here, e.g. what distinguishes this from "

[PATCH] D53587: [clangd] Truncate SymbolID to 16 bytes.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. The goal is 8 bytes, which has a nonzero risk of collisions with huge indexes. This patch should shake out any issues with truncation at

[PATCH] D50147: clang-format: support external styles

sammccall added a comment. The idea here does seem to be a natural extension of -style, at least for the case when the arg is a filename directly. I'm not opposed, happy to review this. I do want to probe the use case a bit though: we have found that configuring via -style= doesn't scale that w

[PATCH] D53587: [clangd] Truncate SymbolID to 16 bytes.

sammccall marked 2 inline comments as done. sammccall added inline comments. Comment at: clangd/index/Serialization.cpp:303 // A refs section has data grouped by Symbol. Each symbol has: -// - SymbolID: 20 bytes +// - SymbolID: 16 bytes // - NumRefs: varint

[PATCH] D53587: [clangd] Truncate SymbolID to 16 bytes.

This revision was automatically updated to reflect the committed changes. Closed by commit rL345113: [clangd] Truncate SymbolID to 16 bytes. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D53587?vs=170684&id=170

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL345119: [clangd] Embed fixes as CodeAction, instead of clangd_fixes. Clean up… (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.l

[PATCH] D53191: [clang] Introduce new completion context types

sammccall added a comment. Logic looks good. Any way we can exercise it with a test via c-index-test? Comment at: include/clang/Sema/CodeCompleteConsumer.h:281 + +/// Code completion occurred where an existing symbol is expected. +CCC_Symbol, nit: try t

[PATCH] D52448: [clang-format] Break before next parameter after a formatted multiline raw string parameter

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: lib/Format/ContinuationIndenter.cpp:1594 StartColumn + NewPrefixSize - Style.ColumnLimit : 0; - return Fixes.second + PrefixExcessCharacters * S

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

sammccall added a comment. All these things are good ideas - I'm not addressing comments yet because I'm waiting for feedback from the team that's trying this out - maybe this needs patches, or maybe it'll never work. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53406 ___

[PATCH] D53641: [clangd] Remove didOpen extraFlags extension.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. This was added in https://reviews.llvm.org/D34947 to support YCM, but YCM actually provides *all* args, and this was never actually use

[PATCH] D53642: [clangd] Don't invalidate LSP-set compile commands when closing a file.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. It doesn't make much sense: setting them is not coupled to opening the file, it's an asynchronous notification. I don't think this is a

[PATCH] D53571: [clangd] Don't show base class versions of members as completions.

sammccall added a comment. In https://reviews.llvm.org/D53571#1274082, @ioeric wrote: > > Keep required quailifier machinery around though, for cross-ns completion. > > Do we have cross-ns completion in sema? No, I don't think so. I meant RequiredQualifier etc. Unfortunately I've got no idea wh

[PATCH] D53571: [clangd] Don't show base class versions of members as completions.

sammccall updated this revision to Diff 170863. sammccall added a comment. Remove misleading change to test. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53571 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/CodeComplete.cpp clangd/GlobalCompilationD

[PATCH] D53638: [clangd] Downrank members from base class

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/Quality.cpp:380 + if (InBaseClass) +Score *= 0.7; + This seems like a pretty light penalty to me, I'd consider 0.5... Rep

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

sammccall updated this revision to Diff 170866. sammccall added a comment. Clarify Span::Args lifetime. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53399 Files: clangd/ClangdLSPServer.cpp clangd/Trace.h Index: clangd/Trace.h =

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

sammccall added inline comments. Comment at: clangd/ClangdLSPServer.cpp:112 +SPAN_ATTACH(Tracer, "Params", Params); +ReplyOnce Reply(ID, Method, &Server, Tracer.Args); log("<-- {0}({1})", Method, ID); ioeric wrote: > Do we have guarantee that `Tracer

[PATCH] D53638: [clangd] Downrank members from base class

sammccall added inline comments. Comment at: clangd/Quality.cpp:380 + if (InBaseClass) +Score *= 0.7; + ioeric wrote: > sammccall wrote: > > This seems like a pretty light penalty to me, I'd consider 0.5... > 0.5 sounds reasonable. I think we should penalize

[PATCH] D53644: [clangd] workspace/symbol should be async, it reads from the index.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, jfb, arphaman, jkorous, MaskRay, ioeric, javed.absar. To enable this, TUScheduler has to provide a way to run async tasks without needing a preamble or AST! Repository:

[PATCH] D53571: [clangd] Don't show base class versions of members as completions.

sammccall updated this revision to Diff 170875. sammccall added a comment. Rebase Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53571 Files: clangd/CodeComplete.cpp unittests/clangd/CodeCompleteTests.cpp Index: unittests/clangd/CodeCompleteTests.cpp =

[PATCH] D53571: [clangd] Don't show base class versions of members as completions.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE345141: [clangd] Don't show base class versions of members as completions. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53571?vs=170875&id=170876#toc Rep

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

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE345144: [clangd] Ensure that we reply to each call exactly once. NFC (I think!) (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53399?vs=170866&id=170881#toc

[PATCH] D53647: [clangd] When replying, log the method name and latency.

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov. This information is strictly available in the log (you can find the original call) but it makes the log easier to follow in practice.

[PATCH] D53647: [clangd] When replying, log the method name and latency.

sammccall added a comment. Log now looks something like this: I[16:47:01.225] <-- textDocument/completion("2") I[16:47:01.233] Ignored diagnostic. /usr/local/google/home/sammccall/temp/test.cc:7:1:'main' must return 'int' I[16:47:01.234] Code complete: sema context Statement, query scopes

[PATCH] D53648: [clangd] Only log ignored diagnostics with -log=verbose.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53648 Files: clangd/Compiler.cpp Index: clangd/Compiler.cpp ===

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

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: unittests/clangd/CodeCompleteTests.cpp:2180 + clangd::CodeCompleteOptions Opts; + Opts.AllScopes = true; + auto Results = completions(R"cpp(

[PATCH] D53191: [clang] Introduce new completion context types

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. (sorry, thought I had accepted this!) Repository: rC Clang https://reviews.llvm.org/D53191 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D53647: [clangd] When replying, log the method name and latency.

sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clangd/ClangdLSPServer.cpp:221 (*TraceArgs)["Reply"] = *Reply; -else { - auto Err = Reply.takeError(); +Server->Transp.reply(std::move(ID), std::move(Reply

[PATCH] D53647: [clangd] When replying, log the method name and latency.

This revision was automatically updated to reflect the committed changes. Closed by commit rL345150: [clangd] When replying, log the method name and latency. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D53647

[PATCH] D53642: [clangd] Don't invalidate LSP-set compile commands when closing a file.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE345231: [clangd] Don't invalidate LSP-set compile commands when closing a file. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53642?vs=170861&id=171033#toc

[PATCH] D53687: [clangd] Make in-memory CDB always available as an overlay, refactor.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. The new implementation is a GlobalCompilationDatabase that overlays a base. Normally this is the directory-based CDB. To preserve the be

[PATCH] D53688: [clangd] Add fallbackFlags initialization extension.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. This allows customizing the flags used when no compile database is available. It addresses some uses of the old extraFlags extension.

[PATCH] D53641: [clangd] Remove didOpen extraFlags extension.

sammccall added a comment. BTW I'm holding off submitting this until we decide the fate of https://reviews.llvm.org/D53688. If we're going to add that extension, I'd like to land the two patches together so the former user of this extension can rely on *one* of them being available. Repository

[PATCH] D53651: [clangd] Use thread pool for background indexing.

sammccall added a comment. In https://reviews.llvm.org/D53651#1275517, @ilya-biryukov wrote: > It's fine to spend one thread spinning on background tasks, but if we're > going to do a threadpool, we should be more careful to not hurt the > performance of foreground tasks. To do that, we should

[PATCH] D53644: [clangd] workspace/symbol should be async, it reads from the index.

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rCTE345268: [clangd] workspace/symbol should be async, it reads from the index. (authored by sammccall, committed by ). Changed prior to commit: https://reviews

[PATCH] D53687: [clangd] Make in-memory CDB always available as an overlay, refactor.

sammccall marked 2 inline comments as done. sammccall added a comment. In https://reviews.llvm.org/D53687#1275580, @ilya-biryukov wrote: > FWIW, the old implementation of the CDB looked simpler (which makes sense, > since it only allowed the in-memory compile commands, but the new > implementat

[PATCH] D53687: [clangd] Make in-memory CDB always available as an overlay, refactor.

sammccall updated this revision to Diff 171098. sammccall marked an inline comment as done. sammccall added a comment. Switch boolean parameters to positive sense, rebase. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53687 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLS

[PATCH] D53688: [clangd] Add fallbackFlags initialization extension.

sammccall added a comment. In https://reviews.llvm.org/D53688#1275586, @ilya-biryukov wrote: > This feels like a configuration option that might be changed in the course of > running clangd. > Are there any strong reasons to make it work only upon initialization? My > guess is that it keeps th

[PATCH] D50147: clang-format: support external styles

sammccall added a comment. In https://reviews.llvm.org/D50147#1279056, @Typz wrote: > In https://reviews.llvm.org/D50147#1272742, @sammccall wrote: > > > The idea here does seem to be a natural extension of -style, at least for > > the case when the arg is a filename directly. I'm not opposed, h

[PATCH] D53808: [clangd] Don't collect refs from non-canonical headers.

sammccall added a comment. It's not obvious whether/why this is the right thing to do. One set of reasoning that occurs to me: - headers without guards produce code in some context-sensitive way, they only exist in the context of their including file - for the symbols they produce, we have ways

[PATCH] D53651: [clangd] Use thread pool for background indexing.

sammccall added inline comments. Comment at: clangd/Threading.cpp:102 +void setThreadPriority(std::thread &T, ThreadPriority Priority) { +#ifdef HAVE_PTHREAD_H + sched_param priority; don't you also need to actually include it? Comment at: cla

[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Great stuff! Just nits (though a few of them; this is a tricky patch!). Comment at: clangd/index/Background.cpp:122 +inline BackgroundIndex::FileDigest digest(StringR

[PATCH] D53651: [clangd] Use thread pool for background indexing.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Nice! Sorry about the back and forth with threadpool... Comment at: clangd/index/Background.cpp:35 + assert(ThreadPoolSize > 0 && "Thread pool size can't be zero."); +

[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.

sammccall accepted this revision. sammccall added inline comments. Comment at: clangd/index/Background.cpp:311 SPAN_ATTACH(Tracer, "refs", int(Refs.numRefs())); - // FIXME: partition the symbols by file rather than TU, to avoid duplication. - IndexedSymbols.update(AbsolutePa

[PATCH] D53922: [clangd] fix non linux build

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks! We should work out what to do on BSD+Mac (and windows at some point), but no-op is good for now. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53922 ___

[PATCH] D53900: [CodeComplete] Penalize inherited ObjC properties for auto-completion

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Looks sane to me, you might want someone more familiar with ObjC to verify though. Repository: rC Clang https://reviews.llvm.org/D53900 ___

[PATCH] D53922: [clangd] fix non linux build

sammccall added a comment. In https://reviews.llvm.org/D53922#1281882, @krytarowski wrote: > For NetBSD: > > - `ThreadPriority::Low` select either `SCHED_RR` or `SCHED_FIFO`, call > sched_get_priority_min() and set pthread_setschedparam(). https://www.netbsd.org/docs/internals/en/chap-processe

[PATCH] D53922: [clangd] fix non linux build

sammccall added a comment. In https://reviews.llvm.org/D53922#1281902, @krytarowski wrote: > In https://reviews.llvm.org/D53922#1281898, @sammccall wrote: > > > In https://reviews.llvm.org/D53922#1281882, @krytarowski wrote: > > > > > For NetBSD: > > > > > > - `ThreadPriority::Low` select either

[PATCH] D55185: [Clangd] Index main-file symbols (bug 39761)

This revision was automatically updated to reflect the committed changes. Closed by commit rL351041: [clangd] Index main-file symbols (bug 39761) (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D55185: [Clangd] Index main-file symbols (bug 39761)

sammccall updated this revision to Diff 181510. sammccall added a comment. Herald added a subscriber: cfe-commits. add comment about subtleties around canonical declarations and main-file check invert sense of Symbol flag (FileLocal -> VisibleOutsideFile) so Merge does the right thing test: add c

[PATCH] D55185: [Clangd] Index main-file symbols (bug 39761)

sammccall added a comment. The forward-declaration-in-main-file case: In principle, if the definition is missing, I think we should treat these differently (not index them, or treat them as visible-outside file) because they're almost certainly declared "properly" elsewhere. @ilya-biryukov convi

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

This revision was automatically updated to reflect the committed changes. Closed by commit rL351047: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://

[PATCH] D56552: [clang-tidy] update FunctionSizeCheck for D56444

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE351048: [clang-tidy] update FunctionSizeCheck for D56444 (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D56552?vs=181082&id=181520#toc Repository: rCTE Cl

[PATCH] D56597: [clangd] Add Limit parameter for xref.

sammccall added inline comments. Comment at: clangd/XRefs.cpp:724 } - + if (!Limit) +Limit = std::numeric_limits::max(); nit: move this to the top, since it's just adjusting input params? Comment at: clangd/XRefs.cpp:730 for (const a

[PATCH] D56267: [clangd] Interfaces for writing code actions

sammccall added a comment. High level comments: - this is an important concept and in hindsight it's amazing we didn't have it yet! - I don't get a clear sense of the boundaries between "code action", "refactoring", and "action". There are also "prepared actions" and "instant actions". I thi

[PATCH] D56665: [AST] Fix double-traversal of code in top-level lambdas in RAV(implicit = yes).

sammccall created this revision. sammccall added reviewers: bkramer, klimek. Herald added a subscriber: cfe-commits. Prior to r351069, lambda classes were traversed or not depending on the {Function, Class, Namespace, TU} DeclContext containing them. If it was a function (common case) they were no

[PATCH] D56665: [AST] Fix double-traversal of code in top-level lambdas in RAV(implicit = yes).

sammccall marked an inline comment as done. sammccall added inline comments. Comment at: unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp:72 + Visitor.ExpectMatch("", 1, 10); + Visitor.ExpectMatch("", 1, 14); + EXPECT_TRUE(Visitor.runOver("auto x = []{ [] {}; };", ---

[PATCH] D56267: [clangd] Interfaces for writing code actions

sammccall added a comment. To be a bit more concrete... One approach which won't win any OO design points but might be quite easy to navigate: combine the ActionProvider and PreparedAction into one class. class SwapIfBranches : public MiniRefactoringThing { SwapIfBranches(); // must have

[PATCH] D56267: [clangd] Interfaces for writing code actions

sammccall added a comment. Name ideas: - "Refactoring" - I think the main drawback is the conflicts with potential global-refactorings that we do in clangd. I don't think the conflict with `tooling/refactor` is bad enough to avoid this. I also don't think "some of these don't look like refacto

[PATCH] D56597: [clangd] Add Limit parameter for xref.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: unittests/clangd/DexTests.cpp:684 + } + { +Req.Limit = 1; instead of splitting scopes here, might be more obvious just to `Files.

[PATCH] D56665: [AST] Fix double-traversal of code in top-level lambdas in RAV(implicit = yes).

This revision was automatically updated to reflect the committed changes. Closed by commit rL351075: [AST] Fix double-traversal of code in top-level lambdas in RAV(implicit = yes). (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINC

[PATCH] D55256: [clangd] Support clang-tidy configuration in clangd.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Nice! I think we should get to `.clang-tidy` files subsequently, but this is a great start and allows us to hook up the right set of checks in other environments. Com

[PATCH] D56267: [clangd] Interfaces for writing code actions

sammccall added a comment. Offline discussion: - CRTP doesn't buy anything. Simple inheritance is less clean than current function-passing version but probably more familiar. - `Tweak` is a good name: short, evocative but not commonly used, works as a noun and a verb. - Registry might be a good

[PATCH] D54428: [clangd] XPC transport layer, framework, test-client

sammccall added a comment. Sorry for not getting back to this, but it looks good. I'm really glad about how minimal the changes are outside xpc/, because it makes it less likely we'll accidentally break something that can only be tested on mac. Repository: rCTE Clang Tools Extra CHANGES SI

[PATCH] D56267: [clangd] Interfaces for writing code actions

sammccall added a comment. (Mostly just comments on Tweak, I think the comments on other parts still apply) Comment at: clangd/SourceCode.h:58 +/// Return the file location, corresponding to \p P. Note that one should take +/// care to avoid comparing the result with expansio

[PATCH] D56841: [clangd] Filter out plugin related flags.

sammccall added a comment. In D56841#1361395 , @kadircet wrote: > It seems like the most relevant place in tooling is ArgumentsAdjuster as > @ioeric pointed out. Happy to move it to there if @sammccall and @klimek > agrees as well. > > As a side note `Ar

[PATCH] D56267: [clangd] Interfaces for writing code actions

sammccall added a comment. This looks really good, everything else is pretty superficial I think. I think we'll want to add one lit test for the whole two-step tweak workflow, as well as TestTU/annotation-based unit-tests for each tweak. As this patch has no actual tweaks in it, we can work out

[PATCH] D56267: [clangd] Interfaces for writing code actions

sammccall added inline comments. Comment at: clangd/ClangdServer.cpp:339 + return CB(llvm::createStringError(llvm::inconvertibleErrorCode(), +"could not create action context")); +CB(prepareTweaks(*Inputs)); (actio

[PATCH] D56611: [clangd] A code action to swap branches of an if statement

sammccall added a comment. Cool! Adopting this one as the simplest tweak for "how should tweaks do X" questions. This depends on helpers not (yet) present in this patch, so not commenting on them now. Need unit tests for tweaks. Something like this should work: Annotations Test(R"cpp(void f

[PATCH] D55256: [clangd] Support clang-tidy configuration in clangd.

sammccall accepted this revision. sammccall added inline comments. Comment at: clangd/tool/ClangdMain.cpp:204 +static llvm::cl::opt ClangTidyChecks( +"clang-tidy-checks", hokein wrote: > sammccall wrote: > > Maybe add a TODO or FIXME to respect .clang-tidy

[PATCH] D56917: [clang] add tests to ExprMutAnalyzer that reproduced a crash in ASTMatchers

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:1112 +TEST(ExprMutationAnalyzerTest, ReproduceFailure11) { + const std::string Reproducer = -

[PATCH] D56267: [clangd] Interfaces for writing code actions

sammccall added a comment. Can live with `applyTweak` etc, though it makes me sad. Comment at: clangd/refactor/Tweak.h:40 + struct Selection { +static llvm::Optional create(llvm::StringRef File, +llvm::StringRef Code, ---

[PATCH] D56860: [clangd] NFC: Use buildCompilerInvocation in CodeComplete

sammccall added a comment. In D56860#1365432 , @ilya-biryukov wrote: > Adding Sam as a reviewer, IIRC we used to have `buildCompilerInvocation` here > and he was the one who inlined it. > I would personally LGTM this change (with the two comments fixed)

[PATCH] D56903: [clangd] Suggest adding missing includes for incomplete type diagnostics.

sammccall added a comment. This looks pretty good! My main concern is latency (and variability of latency) in practice. Particularly: - we should avoid paying for fuzzyfind and fetching hundreds of results when we want exact-match - limit the damage in degenerate cases: should we limit to e.g.

[PATCH] D56860: [clangd] NFC: Use buildCompilerInvocation in CodeComplete

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thank you! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56860/new/ https://reviews.llvm.org/D56860

[PATCH] D57043: [clangd] Use the shared forced-linking code.

sammccall added inline comments. Comment at: clangd/CMakeLists.txt:110 +if( CLANG_ENABLE_STATIC_ANALYZER ) + target_link_libraries(clangDaemon PRIVATE clangd doesn't support the static analyzer. Fundamentally I don't know how to enforce the partial-traversal (

[PATCH] D56924: Handle ObjCCategoryDecl class extensions for print

sammccall added a comment. This is definitely an improvement, though I don't know if it's *right*. @akyrtzi, thoughts? Comment at: unittests/AST/NamedDeclPrinterTest.cpp:206 +"property", +"(class extension)::property")); +} One concern is that similarl

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

sammccall updated this revision to Diff 183291. sammccall added a comment. Actually include the relevant code change :-\ Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57150/new/ https://reviews.llvm.org/D57150 Files: lib/Lex/HeaderSearch.cpp test/PCH/leakfil

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

sammccall added a subscriber: hans. sammccall added a comment. @hans: I think we're going to need either this fix or a revert of rL347205 + dependents cherrypicked for 8.0. I don't know what the constraints/goals are for releases, let me know if you have an op

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

sammccall created this revision. sammccall added reviewers: thakis, ilya-biryukov, bkramer. Herald added a subscriber: cfe-commits. We don't immediately need the file to be open, and this code does not ensure that it is closed. On at least some code paths (using headers from PCHs) the file is neve

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

sammccall added a comment. Sigh, this regresses perf by 6%ish in my testing (building HeaderSearch.cpp with -fsyntax-only and a cold disk cache). I think we'll need a more targeted fix: probably passing openFile=false only in the using-PCH case. Plan for now is to revert the original change, li

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