[PATCH] D54796: [clangd] C++ API for emitting file status

hokein added a comment. Forgot to associate this patch to the actual commit, committed in rL348475 . Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54796/new/ https://reviews.llvm.org/D54796

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein added inline comments. Comment at: clangd/TUScheduler.cpp:268 + /// Status of the TU. + TUStatus Status; /* GUARDED_BY(DiagMu) */ }; ilya-biryukov wrote: > hokein wrote: > > ilya-biryukov wrote: > > > hokein wrote: > > > > ilya-biryukov wrote: > > > >

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein updated this revision to Diff 176936. hokein marked 8 inline comments as done. hokein added a comment. Address comments. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54796/new/ https://reviews.llvm.org/D54796 Files:

[PATCH] D54796: [clangd] C++ API for emitting file status

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, still have a few NITs and suggestions (see the comment about emitting "queued" on waiting for diagnostics and **not** emitting queued if the lock was acquired from the

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein added inline comments. Comment at: clangd/TUScheduler.cpp:268 + /// Status of the TU. + TUStatus Status; /* GUARDED_BY(DiagMu) */ }; ilya-biryukov wrote: > hokein wrote: > > ilya-biryukov wrote: > > > Is `Status` actually ever read from multiple

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein updated this revision to Diff 176588. hokein marked 6 inline comments as done. hokein added a comment. Address review comments - remove Unknown enum type - make TUState only accessed by the worker thread. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION

[PATCH] D54796: [clangd] C++ API for emitting file status

ilya-biryukov added a comment. Sorry for the delays. Not sure about emitting `Queued` on the main thread, see the corresponding comment. Otherwise looks very good. Comment at: clangd/ClangdServer.h:49 + /// Called whenever the file status is updated. + virtual void

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein marked an inline comment as not done. hokein added inline comments. Comment at: clangd/ClangdServer.h:49 + /// Called whenever the file status is updated. + virtual void onFileUpdated(PathRef File, const TUStatus ){}; }; ilya-biryukov wrote: > hokein

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein updated this revision to Diff 176111. hokein marked 12 inline comments as done. hokein added a comment. Address comments and fix a bug in the test. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54796/new/ https://reviews.llvm.org/D54796

[PATCH] D54796: [clangd] C++ API for emitting file status

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.h:49 + /// Called whenever the file status is updated. + virtual void onFileUpdated(PathRef File, const TUStatus ){}; }; hokein wrote: > ilya-biryukov wrote: > > Have we thought about the

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein added inline comments. Comment at: clangd/ClangdServer.h:49 + /// Called whenever the file status is updated. + virtual void onFileUpdated(PathRef File, const TUStatus ){}; }; ilya-biryukov wrote: > Have we thought about the way we might expose

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein updated this revision to Diff 176098. hokein marked 11 inline comments as done. hokein added a comment. Fix nits. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54796/new/ https://reviews.llvm.org/D54796 Files: clangd/ClangdServer.cpp

[PATCH] D54796: [clangd] C++ API for emitting file status

ilya-biryukov added a comment. > @ilya-biryukov, I hope the current patch is not too big for you to review, > happy to chat offline if you want (sam and I had a lot of discussions before > he is OOO). Picking it up. Mostly NITs from my side and a few questions to better understand the design.

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein added a comment. @ilya-biryukov, I hope the current patch is not too big for you to review, happy to chat offline if you want (sam and I had a lot of discussions before he is OOO). Comment at: clangd/ClangdLSPServer.cpp:787 +void ClangdLSPServer::onFileUpdated(const

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein updated this revision to Diff 175859. hokein marked 10 inline comments as done. hokein added a comment. - address review comments - drop the LSP change, only focus on TUScheduler in this patch Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION

[PATCH] D54796: [clangd] C++ API for emitting file status

sammccall added inline comments. Comment at: clangd/ClangdLSPServer.cpp:787 +void ClangdLSPServer::onFileUpdated(const FileStatus ) { + notify("window/showMessage", FStatus); +} notifying via `showMessage` looks sensible at first glance as a fallback. Later we

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein added a comment. > It would be reasonable to be consistent with diagnostics: stop emitting the > statuses when ASTWorker was put into shutdown mode. Thanks, sounds fair. Comment at: clangd/ClangdServer.h:39 +// FIXME: find a better name. class DiagnosticsConsumer {

[PATCH] D54796: [clangd] C++ API for emitting file status

hokein updated this revision to Diff 175277. hokein marked 2 inline comments as done. hokein added a comment. Herald added a subscriber: jfb. stop emitting file status when ASTworker is shutting down. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION

[PATCH] D54796: [clangd] C++ API for emitting file status

ilya-biryukov added a comment. In D54796#1306960 , @hokein wrote: > There is one thing I'm not certain: should we stop emitting the file status > when the file is removed (similar to the behavior of diagnostics)? For > example, the file is removed while

[PATCH] D54796: [clangd] C++ API for emitting file status

jkorous added a comment. Hi @hokein, I am just keeping up to date with changes. Comment at: clangd/ClangdServer.h:39 +// FIXME: find a better name. class DiagnosticsConsumer { It would be unfortunate to have this name clashing with