[PATCH] D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

sammccall created this revision. sammccall added reviewers: bkramer, thakis. Herald added subscribers: cfe-commits, kadircet, ioeric, ilya-biryukov. r347205 fixed a bug in FileManager: first calling getFile(shouldOpen=false) and then getFile(shouldOpen=true) results in the file not being open. Un

[PATCH] D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

sammccall updated this revision to Diff 183340. sammccall marked an inline comment as done. sammccall added a comment. avoid revert of fillRealPathName() (thanks Nico!) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57165/new/ https://reviews.llvm.org/D57165 File

[PATCH] D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

This revision was automatically updated to reflect the committed changes. Closed by commit rC352079: [FileManager] Revert r347205 to avoid PCH file-descriptor leak. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D57165?vs=183340&id=183347#toc Reposito

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

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Just a bunch of nits, up to you to work out how/whether to address. One substantive issue around classes defined outside headers - will chat offline. Comment at: clan

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

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/Protocol.cpp:425 +const llvm::StringLiteral ExecuteCommandParams::CLANGD_APPLY_TWEAK = +"clangd.applyCodeAction"; + ---

[PATCH] D57057: [clangd] Log clang-tidy configuration, NFC

sammccall added a comment. This turns out to be excessively spammy in unit tests, can we change this to dlog()? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57057/new/ https://reviews.llvm.org/D57057 ___ cfe-commits

[PATCH] D57388: [clangd] Implement textDocument/declaration from LSP 3.14

sammccall created this revision. sammccall added a reviewer: hokein. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric, javed.absar, ilya-biryukov. LSP now reflects the declaration/definition distinction. Language server changes: - textDocument/definition now r

[PATCH] D57388: [clangd] Implement textDocument/declaration from LSP 3.14

sammccall updated this revision to Diff 184096. sammccall added a comment. Add server capability declarationProvider. Technically an extension, but that's probably just a protocol bug. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57388/new/ https:/

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

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/SourceCode.h:63 +/// Turns a token range into a half-open range and checks its correctness. +/// The resulting range will hav

[PATCH] D57509: [clangd] Append "(fix available)" to diagnostic message when fixes are present.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. In D57509#1378696 , @kadircet wrote: > Dropping by: Maybe add `(fix available)` as a prefix rather than suffix. > Since long texts might end up g

[PATCH] D57021: [clangd] Suggest adding missing includes for typos (like include-fixer).

sammccall added inline comments. Comment at: clangd/IncludeFixer.cpp:74 +break; + default: +assert(RecordTypo); "default" looks a bit scary - could still end up with false positives. Can we cover a few of the most common cases (even if we know they're no

[PATCH] D57509: [clangd] Append "(fix available)" to diagnostic message when fixes are present.

sammccall added a comment. FWIW, I don't think this is worth adding an option or protocol extensions for at this point. There are pros and cons to showing this in VSCode (pros are visibility without hovering, particularly in problems view and cross-editor consistency, cons are certainly noise

[PATCH] D53935: Delete dependency on config.h

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/Threading.cpp:7 #include -#ifdef HAVE_PTHREAD_H +#if defined(__USE_POSIX) #include nit: just ifdef? Repository: rCTE Cla

[PATCH] D53935: Delete dependency on config.h

sammccall added inline comments. Comment at: clangd/Threading.cpp:104 void setThreadPriority(std::thread &T, ThreadPriority Priority) { -#if defined(HAVE_PTHREAD_H) && defined(__linux__) +#if defined(__USE_POSIX) && defined(__linux__) sched_param priority; th

[PATCH] D53935: Delete dependency on config.h

sammccall added a comment. In https://reviews.llvm.org/D53935#1282303, @lebedev.ri wrote: > How does this play with `LLVM_ENABLE_THREADS` cmake option? Clangd doesn't support/respect that, and I don't think anyone's building in that configuration. If this causes actual problems e.g. with bots,

[PATCH] D53936: [clang-tidy] More clearly separate public, check-facing APIs from internal ones.

sammccall created this revision. sammccall added a reviewer: hokein. Herald added subscribers: cfe-commits, xazax.hun. This codifies the mostly-current state: - The stuff in ClangTidy.h is reasonable for checks to depend on. - The stuff in ClangTidyDiagnosticConsumer.h is a mish-mash of clang-tid

[PATCH] D53936: [clang-tidy] More clearly separate public, check-facing APIs from internal ones.

sammccall updated this revision to Diff 171937. sammccall added a comment. Add FIXME to fuchsia check that uses private APIs. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53936 Files: clang-tidy/ClangTidy.cpp clang-tidy/ClangTidy.h clang-tidy/ClangTidyDiagnosticConsumer

[PATCH] D53936: [clang-tidy] More clearly separate public, check-facing APIs from internal ones.

sammccall added inline comments. Comment at: clang-tidy/ClangTidy.h:11 +// +// It should remain as stable as possible, as many out-of-tree checks exist. +//===--===// steveire wrote: > Clang C++

[PATCH] D53946: [clangd] Signal when skipping the diagnostic rebuilds.

sammccall added a comment. Discussed offline a bit. We should be judicious with intrusive changes to support out-of-tree clients, and this interface seems a bit messy. There are alternatives with advantages: 1. less intrusive: clients can create a context before calling addDocument and observe

[PATCH] D53953: [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC

sammccall created this revision. sammccall added a reviewer: hokein. Herald added subscribers: cfe-commits, xazax.hun. Currently ClangTidyContext::diag() sends the diagnostics to a DiagnosticsEngine, which probably delegates to a ClangTidyDiagnosticsConsumer, which is supposed to go back and popul

[PATCH] D53953: [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC

sammccall added a comment. @hokein @alexfh There is a test that asserts clang-tidy exits with 0 when processing a nonexistent file (test added in https://reviews.llvm.org/D17335 which you wrote/reviewed). This seems like a bug to me, do I need to preserve this behavior? (See patch description f

[PATCH] D53946: [clangd] Test Context can be used for file status. NFC

sammccall added inline comments. Comment at: unittests/clangd/ClangdTests.cpp:1037 +TEST(ClangdTests, AddDocumentTracksDiagnostics) { + // We have clients relying on the fact that the Context passed to addDocument this tests a lot of things, and after 10 minut

[PATCH] D53953: [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC

sammccall updated this revision to Diff 172039. sammccall added a comment. Remove backwards-compat behavior around ClangdDiagnosticConsumer::finish(), it's presumably a bug Update test to verify we fail but don't crash in that case. auto -> real type Repository: rCTE Clang Tools Extra https:

[PATCH] D53953: [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC

sammccall marked an inline comment as done. sammccall added a comment. Thanks Alex. @hokein, let me know if any concerns. In https://reviews.llvm.org/D53953#1283044, @alexfh wrote: > > error: unable to handle compilation, expected exactly one compiler job in > > '' [clang-diagnostic-error] > >

[PATCH] D53953: [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC

sammccall updated this revision to Diff 172047. sammccall added a comment. Removed unused var Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53953 Files: clang-tidy/ClangTidy.cpp clang-tidy/ClangTidy.h clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tidy/ClangTidyDiagn

[PATCH] D53953: [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC

sammccall marked an inline comment as done. sammccall added a comment. BTW, the missing "X doesn't exist" is because Tooling.cpp turns off driver diagnostics for nonexistent files, because clang/driver wasn't VFS-aware... in 2011. I'll send patches to fix this if there isn't too much fallout (Dr

[PATCH] D53958: [Tooling] Produce diagnostics for missing input files.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added a subscriber: cfe-commits. This was disabled way back in 2011, in the dark times before Driver was VFS-aware. Also, make driver more VFS-aware :-) This breaks one ClangTidy test (we improved the error message), wi

[PATCH] D53933: [clangd] Get rid of QueryScopes.empty() == AnyScope special case.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Yay, thanks! Comment at: clangd/index/Index.h:463 /// The global scope is "", a top level scope is "foo::", etc. - /// FIXME: drop the special case for empty list,

[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:194 - // FIXME: partition the symbols by file rather than TU, to avoid duplication. - IndexedSymbols.update(AbsolutePath, -llvm::make_unique(std::m

[PATCH] D53936: [clang-tidy] More clearly separate public, check-facing APIs from internal ones.

sammccall updated this revision to Diff 172058. sammccall added a comment. Rethink comment about public API stability. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53936 Files: clang-tidy/ClangTidy.cpp clang-tidy/ClangTidy.h clang-tidy/ClangTidyDiagnosticConsumer.cpp

[PATCH] D53936: [clang-tidy] More clearly separate public, check-facing APIs from internal ones.

sammccall added a comment. Fair enough. I think it's worth saying *something* here, as the stability of these API has value and churning them has costs that far exceed (2 of magnitude?) the other C++ APIs in clang-tidy. And out-of-tree dependencies are something I usually want to be aware of wh

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

sammccall added inline comments. Comment at: clangd/index/Background.cpp:194 - // FIXME: partition the symbols by file rather than TU, to avoid duplication. - IndexedSymbols.update(AbsolutePath, -llvm::make_unique(std::move(Symbols)), ka

[PATCH] D53953: [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC

sammccall marked an inline comment as done. sammccall added a comment. In https://reviews.llvm.org/D53953#1285244, @hokein wrote: > I think this is a bug, we should return non-zero code when processing a > non-existing file. The new behavior seems better to me, though we need to > polish the er

[PATCH] D53953: [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC

sammccall updated this revision to Diff 172326. sammccall added a comment. Address comment, rebase Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53953 Files: clang-tidy/ClangTidy.cpp clang-tidy/ClangTidy.h clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tidy/ClangTidy

[PATCH] D53953: [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE345961: [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53953?vs=172326&id=1723

[PATCH] D54033: [clang-tidy] Untangle layering in ClangTidyDiagnosticConsumer somewhat. NFC

sammccall created this revision. sammccall added a reviewer: hokein. Herald added subscribers: cfe-commits, xazax.hun. Clang's hierarchy is CompilerInstance -> DiagnosticsEngine -> DiagnosticConsumer. (Ownership is optional/shared, but this structure is fairly clear). Currently ClangTidyDiagnost

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL345968: [clangd] Only log ignored diagnostics with -log=verbose. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D53648 Fi

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

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE345970: [clangd] Make in-memory CDB always available as an overlay, refactor. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53687?vs=171098&id=172342#toc

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

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE345969: [clangd] Remove didOpen extraFlags extension. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53641?vs=170858&id=172341#toc Repository: rCTE Clang

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL345970: [clangd] Make in-memory CDB always available as an overlay, refactor. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.ll

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

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE345973: [clangd] Add fallbackFlags initialization extension. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53688?vs=171036&id=172351#toc Repository: rCT

[PATCH] D54036: [fix][clang-tidy] fix for r345961 that introduced a test failure on Windows builds

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thank you! `{{.*}}not-exist` also seems fine, up to you. Repository: rL LLVM https://reviews.llvm.org/D54036 ___ cfe-commits mailing li

[PATCH] D54061: [clang-tidy] run() doesn't update the SourceManager.

sammccall created this revision. sammccall added reviewers: hokein, alexfh. Herald added subscribers: cfe-commits, xazax.hun. By now the context's SourceManager is now initialized everywhere that ClangTidyCheck::registerMatcher() is called, so the call from run() seems entirely redundant, and inde

[PATCH] D54077: [clangd] Implemented DraftFileSystem

sammccall added subscribers: ilya-biryukov, LutsenkoDanil. sammccall added a comment. Disclaimer: I'm on a train with family today, and haven't actually read the patch... So I have concerns :-) 1. There's the usual concern that the current behavior is reasonable and people like it, so adding a

[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files

sammccall added a comment. In https://reviews.llvm.org/D51729#1287421, @Lekensteyn wrote: > Before this patch, missing compilation database entries resulted in "Skipping > Compile command not found." which is assumed by the tests in this > clang-query patch: https://reviews.llvm.org/D54109

[PATCH] D54077: [clangd] Implemented DraftFileSystem

sammccall added a comment. I don't think either point 1 or 2 above have been addressed, and so I'm not comfortable moving forward with this change. > I think it would be reasonable to say that a large portion of C++ users are > used to the behavior that this patch introduces. I agree, the new

[PATCH] D54077: [clangd] Implemented DraftFileSystem

sammccall added a comment. Someone mentioned to me that the interaction-between-features argument wasn't clear here: - we **don't** currently update diagnostics for `A.cc` when `A.h` is edited - we should, this seems more obvious & important than what we do with drafts - this interacts badly wit

[PATCH] D54061: [clang-tidy] run() doesn't update the SourceManager.

sammccall added a comment. In https://reviews.llvm.org/D54061#1286956, @JonasToth wrote: > > Theoretically, we could replace `ClangTidyCheck::check` with > > `ClangTidyCheck::run`, but I'm not sure it is worth, > > `ClangTidyCheck::check` is a public API, and is widely-used (for all > > clang-

[PATCH] D54061: [clang-tidy] run() doesn't update the SourceManager.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE346219: [clang-tidy] run() doesn't update the SourceManager. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D54061?vs=172471&id=172719#toc Repository: rCT

[PATCH] D54077: [clangd] Implemented DraftFileSystem

sammccall added a comment. In https://reviews.llvm.org/D54077#1288455, @LutsenkoDanil wrote: > I already made a patch which introduces such behavior (not uploaded here > yet), and looks like it works well with draft fs: diagnostics updates for > depended files in 'real-time' on typing for opene

[PATCH] D54156: [CodeComplete] Do not complete self-initializations

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. :-) Repository: rC Clang https://reviews.llvm.org/D54156 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D54204: [clangd] Initial clang-tidy diagnostics support.

sammccall created this revision. sammccall added a reviewer: hokein. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov, mgorny, srhines. This runs checks over a restricted subset of the TU: - preprocessor callbacks just receive the truncated PP ev

[PATCH] D53934: [clangd] Improve code completion for ObjC methods

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks for fixing this! Comment at: unittests/clangd/CodeCompleteTests.cpp:2195 +TEST(CompletionTest, ObjectiveCMethodNoArguments) { + std::string Context = R"objc( +

[PATCH] D54261: [clangd] proof-of-concept: clang-tidy only runs over main-file

sammccall created this revision. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov, kbarton, nemanjai. based on https://reviews.llvm.org/D54259, with API updates to make everything build again. Not ready for review, probably won't become ready in

[PATCH] D53958: [Tooling] Produce diagnostics for missing input files.

This revision was automatically updated to reflect the committed changes. Closed by commit rC346414: [Tooling] Produce diagnostics for missing input files. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53958?vs=172052&id=173180#toc Repository: rC

[PATCH] D54033: [clang-tidy] Untangle layering in ClangTidyDiagnosticConsumer somewhat. NFC

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE346418: [clang-tidy] Untangle layering in ClangTidyDiagnosticConsumer somewhat. NFC (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D54033?vs=172339&id=17318

[PATCH] D54303: [clangd] Don't treat top-level decls as "local" if they are from the preamble.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. These get passed to HandleTopLevelDecl() if they happen to have been deserialized for any reason. We don't want to treat them as part of

[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

sammccall created this revision. sammccall added reviewers: klimek, ioeric. Herald added subscribers: cfe-commits, mgorny. The goal is to allow analyses such as clang-tidy checks to run on a subset of the AST, e.g. "only on main-file decls" for interactive tools. Today, these become "problematica

[PATCH] D54303: [clangd] Don't treat top-level decls as "local" if they are from the preamble.

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rCTE346503: [clangd] Don't treat top-level decls as "local" if they are from the preamble. (authored by sammccall, committed by ). Changed prior to commit: htt

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. This is hacky, but solves an important problem. If you know of a good reviewer for this, you may want a second opinion! Comment at: include/clang/Lex/HeaderSearchOptio

[PATCH] D54204: [clangd] Initial clang-tidy diagnostics support.

sammccall updated this revision to Diff 173357. sammccall marked an inline comment as done. sammccall added a comment. Address comments and rebase on https://reviews.llvm.org/D54309, addressing performance issues. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54204 Files: c

[PATCH] D54204: [clangd] Initial clang-tidy diagnostics support.

sammccall added inline comments. Comment at: clangd/ClangdUnit.cpp:175 +CTFactories.createChecks(CTContext.getPointer(), CTChecks); +for (const auto &Check : CTChecks) { + Check->registerPPCallbacks(*Clang); hokein wrote: > Maybe add the check names

[PATCH] D54269: Introduce shard storage to auto-index.

sammccall added inline comments. Comment at: clangd/index/Background.cpp:252 + +auto Hash = FilesToUpdate.lookup(Path); +// Put shards into storage for subsequent use. nit: i'd suggest doing the writes *after* updating the index, as the latter is user-fa

[PATCH] D54311: Add a test checking clang-tidy can find libc++ on Mac

sammccall added inline comments. Comment at: test/clang-tidy/clang-tidy-mac-libcxx.cpp:11 +// Pretend clang is installed beside the mock library that we provided. +// RUN: echo '[{"directory":"%t","command":"%t/mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -

[PATCH] D54311: Add a test checking clang-tidy can find libc++ on Mac

sammccall added inline comments. Comment at: test/clang-tidy/clang-tidy-mac-libcxx.cpp:13 +// RUN: cp "%s" "%t/test.cpp" +// RUN: clang-tidy "%t/test.cpp" + This should check a diagnostic rather than rely on the error code, I think Repository: rCTE Clang Tool

[PATCH] D54311: Add a test checking clang-tidy can find libc++ on Mac

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: test/clang-tidy/clang-tidy-mac-libcxx.cpp:11 +// Pretend clang is installed beside the mock library that we provided. +// RUN: echo '[{"directory":"%t",

[PATCH] D54269: Introduce shard storage to auto-index.

sammccall added inline comments. Comment at: clangd/index/Background.cpp:252 + +auto Hash = FilesToUpdate.lookup(Path); +// Put shards into storage for subsequent use. kadircet wrote: > sammccall wrote: > > nit: i'd suggest doing the writes *after* updati

[PATCH] D52273: [clangd] Initial implementation of expected types

sammccall added a comment. Forgot to say - the scope here looks just right, thanks for slimming this down! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52273 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llv

[PATCH] D52273: [clangd] Initial implementation of expected types

sammccall added inline comments. Comment at: clangd/ExpectedTypes.cpp:12 + +static llvm::Optional toEquivClass(ASTContext &Ctx, QualType T) { + if (T.isNull() || T->isDependentType()) returning QualType vs Type*? It seems we strip all qualifiers, seems clearest

[PATCH] D54269: Introduce shard storage to auto-index.

sammccall added inline comments. Comment at: clangd/index/Background.cpp:52 +llvm::SmallString<128> +getShardPathFromFilePath(llvm::SmallString<128> ShardRoot, + llvm::StringRef FilePath) { nit: these micro-optimizations with SmallString s

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

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/index/Index.h:85 + assert(L.FileURI && R.FileURI); + int Cmp = std::strcmp(L.FileURI, R.FileURI); + return Cmp == 0 && std::tie(L.Start, L.End

[PATCH] D54475: [clangd] Allow observation of changes to global CDBs.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. Currently, changes *within* CDBs are not tracked (CDB has no facility to do so). However, discovery of new CDBs are tracked (all files a

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

sammccall added inline comments. Comment at: clangd/index/Index.h:306 +llvm::StringRef S(P); +CB(S); +P = S.data(); hokein wrote: > sammccall wrote: > > ```assert (!S.data()[S.size()] && "Visited StringRef must be > > null-terminated") > Does this as

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

sammccall added a comment. A question about the high-level build target setup (I don't know much about XPC services/frameworks, bear with me...): This is set up so that the clangd binary (ClangdMain) can run unmodified as an XPC service, all flags and options are still respected etc. At the sam

[PATCH] D53934: [clangd] Improve code completion for ObjC methods

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE346836: [clangd] Improve code completion for ObjC methods (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D53934?vs=173737&id=173997#toc Repository: rCTE C

[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

sammccall updated this revision to Diff 174000. sammccall marked 2 inline comments as done. sammccall added a comment. Address review comments. Remove special case for TUDecl since no-parents is now handled. Repository: rC Clang https://reviews.llvm.org/D54309 Files: include/clang/AST/ASTC

[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

This revision was automatically updated to reflect the committed changes. sammccall marked 2 inline comments as done. Closed by commit rC346847: [AST] Allow limiting the scope of common AST traversals (getParents, RAV). (authored by sammccall, committed by ). Changed prior to commit: https://re

[PATCH] D54269: Introduce shard storage to auto-index.

sammccall added a comment. Thanks, mostly interface tweaks now. Comment at: clangd/index/Background.cpp:187 + BackgroundIndexStorage *IndexStorage = getIndexStorage(Directory); + if (!IndexStorage) +elog("No index storage for: {0}", Directory); I think it

[PATCH] D54475: [clangd] Allow observation of changes to global CDBs.

sammccall updated this revision to Diff 174014. sammccall marked 3 inline comments as done. sammccall added a comment. Address comments. Add missing OverlayCDB->Base watching (oops!) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54475 Files: clangd/Function.h clangd/Global

[PATCH] D54269: Introduce shard storage to auto-index.

sammccall added a comment. just some nits. Main thing is the LoggingStorage - again I think this may have been a misunderstanding. Comment at: clangd/index/Background.cpp:76 +else + elog("Error while reading shard {0}: {1}", ShardIdentifier, + I.takeError())

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

sammccall added inline comments. Comment at: clang-tools-extra/trunk/clangd/index/Index.h:71 + // via llvm::StringRef. + const char *FileURI = ""; }; alexfh wrote: > hokein wrote: > > alexfh wrote: > > > If the users of the SymbolLocation have a way to get a c

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

sammccall added a comment. Hi Jan, Clangd uses SymbolIDs rather than USRs, to identify symbols. However these are used only internally, and for extension point APIs (SymbolIndex), and not actually exposed over the wire. Can you explain more about the need to identify the symbol in go-to-definit

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

sammccall added a comment. If we're extending the API, we should be careful to do it in an orthogonal and reasonable general way. Apple may not need USR in other places now, but there are users other than Apple and times other than today :-) One of the reasons this looks odd is the method is "f

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

sammccall added a comment. @benlangmuir: I hadn't seen your comment while replying, sorry for any confusion... > The most critical piece of this is being able to ask "what symbol is at this > location" and correlate that with index queries. This makes sense (at least doing follow-up index quer

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

sammccall added a comment. OK, sounds like we have something to move forward with. I'd suggest we start with an operation returning {SymbolID, scope qualifiers, unqualified name, USR} and ignoring location for now, unless you have an immediate need. Reason being this sidesteps the index questio

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

sammccall added a comment. As mentioned offline - sounds like someone from Apple might take a look. If not, ping me again to land! (Sorry for the delay) Repository: rC Clang https://reviews.llvm.org/D53900 ___ cfe-commits mailing list cfe-commit

[PATCH] D54269: Introduce shard storage to auto-index.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks! Looks good, rest of the nits are obvious or up to you.( Comment at: clangd/index/Background.cpp:37 -BackgroundIndex::BackgroundIndex(Context BackgroundContext

[PATCH] D54579: [clang-tidy] Update checks to play nicely with limited traversal scope added in r346847

sammccall created this revision. sammccall added a reviewer: hokein. Herald added subscribers: cfe-commits, xazax.hun. (See https://reviews.llvm.org/D54204 for original review) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54579 Files: clang-tidy/misc/UnusedParametersCheck.c

[PATCH] D54579: [clang-tidy] Update checks to play nicely with limited traversal scope added in r346847

sammccall updated this revision to Diff 174203. sammccall added a comment. Address comments from the last round of review in https://reviews.llvm.org/D54204. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54579 Files: clang-tidy/misc/UnusedParametersCheck.cpp clang-tidy/mo

[PATCH] D54204: [clangd] Initial clang-tidy diagnostics support.

sammccall added a comment. Moved the clang-tidy changes to https://reviews.llvm.org/D54579. Sorry for mixing everything up! Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:518 void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) { - Finder->add

[PATCH] D54204: [clangd] Initial clang-tidy diagnostics support.

sammccall updated this revision to Diff 174204. sammccall added a comment. Remove clang-tidy changes, add FIXME comment. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54204 Files: clangd/CMakeLists.txt clangd/ClangdUnit.cpp clangd/XRefs.cpp unittests/clangd/ClangdUnitT

[PATCH] D54579: [clang-tidy] Update checks to play nicely with limited traversal scope added in r346847

This revision was automatically updated to reflect the committed changes. Closed by commit rL346961: [clang-tidy] Update checks to play nicely with limited traversal scope added in… (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https:/

[PATCH] D54204: [clangd] Initial clang-tidy diagnostics support.

This revision was automatically updated to reflect the committed changes. Closed by commit rL347036: [clangd] Initial clang-tidy diagnostics support. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D54204 Files: c

[PATCH] D54691: [FileManager] getFile(open=true) after getFile(open=false) should open the file.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, ioeric. Old behavior is to just return the cached entry regardless of opened-ness. That feels buggy (though I guess nobody ever actually needed this). This came up in the c

[PATCH] D54694: [clangd] Replay preamble #includes to clang-tidy checks.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. This is needed to correctly handle checks that use IncludeInserter, which is very common. I couldn't find a totally safe example of a c

[PATCH] D54691: [FileManager] getFile(open=true) after getFile(open=false) should open the file.

sammccall marked 3 inline comments as done. sammccall added a comment. In https://reviews.llvm.org/D54691#1302622, @ilya-biryukov wrote: > Overall LG. The only concerning bit is that the initialization of the `UFE` > is now scattered across the function body. > Grouping `File` and `DeferredOpen

[PATCH] D54691: [FileManager] getFile(open=true) after getFile(open=false) should open the file.

sammccall updated this revision to Diff 174577. sammccall added a comment. Address review comments. Repository: rC Clang https://reviews.llvm.org/D54691 Files: include/clang/Basic/FileManager.h lib/Basic/FileManager.cpp unittests/Basic/FileManagerTest.cpp Index: unittests/Basic/FileMa

[PATCH] D54691: [FileManager] getFile(open=true) after getFile(open=false) should open the file.

This revision was automatically updated to reflect the committed changes. Closed by commit rC347205: [FileManager] getFile(open=true) after getFile(open=false) should open the file. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D54691?vs=174577&id=17

[PATCH] D54694: [clangd] Replay preamble #includes to clang-tidy checks.

sammccall updated this revision to Diff 174621. sammccall marked 4 inline comments as done. sammccall added a comment. Address comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54694 Files: clangd/ClangdUnit.cpp clangd/Diagnostics.cpp clangd/Headers.cpp clangd/He

[PATCH] D54694: [clangd] Replay preamble #includes to clang-tidy checks.

sammccall added inline comments. Comment at: clangd/ClangdUnit.cpp:159 + + PP.getPPCallbacks()->InclusionDirective( + HashTok.getLocation(), IncludeTok, WrittenFilename, Angled, ilya-biryukov wrote: > This should be `Delegate->` instead of `PP.getCa

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