[PATCH] D39571: [clangd] DidChangeConfiguration Notification

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL325784: [clangd] DidChangeConfiguration Notification (authored by simark, committed by ). Herald added a subscriber:

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov accepted this revision. ilya-biryukov added a comment. Thanks for fixing all the comments! LGTM! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D39571 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark updated this revision to Diff 135229. simark added a comment. New version Address comments in https://reviews.llvm.org/D39571#1014237 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D39571 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark marked an inline comment as done. simark added inline comments. Comment at: unittests/clangd/ClangdTests.cpp:492 + + EXPECT_TRUE(DiagConsumer.contains(FooCpp)); + EXPECT_TRUE(DiagConsumer.contains(BarCpp)); simark wrote: > ilya-biryukov wrote: > > Maybe

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark marked 5 inline comments as done. simark added inline comments. Comment at: unittests/clangd/ClangdTests.cpp:492 + + EXPECT_TRUE(DiagConsumer.contains(FooCpp)); + EXPECT_TRUE(DiagConsumer.contains(BarCpp)); ilya-biryukov wrote: > Maybe expose a copy of

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: unittests/clangd/ClangdTests.cpp:83 +// least one error. +class MultipleErrorCHeckingDiagConsumer : public DiagnosticsConsumer { +public: NIT: misspelling: ErrorCHecking instead of ErrorChecking

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark updated this revision to Diff 135153. simark added a comment. New version, take 2 The previous version contains the changes of already merged patches, I'm not sure what I did wrong. This is another try. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D39571 Files:

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added a comment. In https://reviews.llvm.org/D39571#1011877, @ilya-biryukov wrote: > > That looks like another preamble-handling bug. It's much easier to fix and > > it's clangd-specific, so I'll make sure to fix that soon. > > Thanks for bringing this up, we haven't been testing

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added a comment. In https://reviews.llvm.org/D39571#1011842, @ilya-biryukov wrote: > In https://reviews.llvm.org/D39571#1007393, @simark wrote: > > > If I do it in this order, I get one diagnostic both times, when I don't > > expect one the second time the file is parsed. But if

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added a comment. In https://reviews.llvm.org/D39571#1007393, @simark wrote: > If I do it in this order, I get one diagnostic both times, when I don't > expect one the second time the file is parsed. But if I do it the other way > (first parse with no errors, second parse with an

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added a comment. In https://reviews.llvm.org/D39571#1007291, @ilya-biryukov wrote: > It looks like a bug in the preamble handling. (It does not check if macros > were redefined). > You can workaround that by making sure the preamble ends before your code > starts (preamble only

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added a comment. In https://reviews.llvm.org/D39571#1006667, @simark wrote: > It seems to me like > > changes in command line arguments (adding -DMACRO=1) are not taken into > account > when changing configuration. It looks like a bug in the preamble handling. (It does not

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark updated this revision to Diff 134097. simark added a comment. Add tests, work in progress Can you take a look at the "TEST(DidChangeConfiguration, DifferentDeclaration)" test, and tell me if you see anything wrong with it? It seems to me like changes in command line arguments (adding

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added a comment. I think I managed to make some tests by using the `MockCompilationDatabase`. Basically with some code like: #ifndef MACRO static void func () {} // 1 #else static void func () {} // 2 #endif and these steps: 1. Server.addDocument(...) 2.

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added a comment. In https://reviews.llvm.org/D39571#1005926, @malaperle wrote: > I haven't looked at the newest patch yet but I gave it a quick try and saw > something odd. If I change the configuration to something invalid (say I > specify the path to a CMakeLists.txt), then I

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

malaperle added a comment. I haven't looked at the newest patch yet but I gave it a quick try and saw something odd. If I change the configuration to something invalid (say I specify the path to a CMakeLists.txt), then I get many errors/diagnostics, which is normal. But then when I change the

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark updated this revision to Diff 133034. simark added a comment. Fix assertion about parsing a document that is not open As found by Ilya, the getActiveFiles method would return the documents that were previously opened and then closed. Repository: rCTE Clang Tools Extra

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:302 +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( simark wrote: > ilya-biryukov wrote: > > simark wrote: > > > simark wrote: > > >

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added inline comments. Comment at: clangd/ClangdLSPServer.cpp:302 +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( ilya-biryukov wrote: > simark wrote: > > simark wrote: > > > ilya-biryukov wrote: > > >

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:302 +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( simark wrote: > simark wrote: > > ilya-biryukov wrote: > > > simark wrote: > > >

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added inline comments. Comment at: clangd/ClangdLSPServer.cpp:302 +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( simark wrote: > ilya-biryukov wrote: > > simark wrote: > > > simark wrote: > > > >

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added inline comments. Comment at: clangd/ClangdLSPServer.cpp:302 +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( ilya-biryukov wrote: > simark wrote: > > simark wrote: > > > ilya-biryukov wrote: > > >

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:78 {"documentHighlightProvider", true}, +{"configurationChangeProvider", true}, {"renameProvider", true}, simark wrote: > ilya-biryukov wrote: > >

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added inline comments. Comment at: clangd/ClangdLSPServer.cpp:78 {"documentHighlightProvider", true}, +{"configurationChangeProvider", true}, {"renameProvider", true}, ilya-biryukov wrote: > simark wrote: > > Nebiroth

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added inline comments. Comment at: clangd/ClangdLSPServer.cpp:302 +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( ilya-biryukov wrote: > simark wrote: > > ilya-biryukov wrote: > > > simark wrote: > > >

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:78 {"documentHighlightProvider", true}, +{"configurationChangeProvider", true}, {"renameProvider", true}, simark wrote: > Nebiroth wrote: > > simark

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added inline comments. Comment at: clangd/ClangdLSPServer.cpp:302 +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( ilya-biryukov wrote: > simark wrote: > > ilya-biryukov wrote: > > > Are you planning to

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:302 +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( simark wrote: > ilya-biryukov wrote: > > Are you planning to to address this

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark updated this revision to Diff 131305. simark added a comment. Move implementation of setCompileCommandsDir to .cpp Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D39571 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added inline comments. Comment at: clangd/ClangdLSPServer.cpp:302 +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( ilya-biryukov wrote: > Are you planning to to address this FIXME before checking the

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added a comment. Hi @simark , thanks for picking up this change. Comment at: clangd/ClangdLSPServer.cpp:302 +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( Are you planning to to address this

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark updated this revision to Diff 131131. simark added a comment. Fix formatting, remove capability - Fix some formatting nits - Remove the entry in the capabilities object Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D39571 Files: clangd/ClangdLSPServer.cpp

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added inline comments. Comment at: clangd/ClangdLSPServer.cpp:78 {"documentHighlightProvider", true}, +{"configurationChangeProvider", true}, {"renameProvider", true}, Nebiroth wrote: > simark wrote: > > I find

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Nebiroth added inline comments. Comment at: clangd/ClangdLSPServer.cpp:78 {"documentHighlightProvider", true}, +{"configurationChangeProvider", true}, {"renameProvider", true}, simark wrote: > I find

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark added inline comments. Comment at: clangd/ClangdLSPServer.cpp:78 {"documentHighlightProvider", true}, +{"configurationChangeProvider", true}, {"renameProvider", true}, I find `configurationChangeProvider` a bit weird.

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

simark updated this revision to Diff 131051. simark added a comment. Herald added subscribers: ioeric, jkorous-apple. I just got familiar with the patch, I cleaned up the bits that I thought were unnecessary for this change. For example, we don't need a toJSON function for

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Nebiroth updated this revision to Diff 126980. Nebiroth marked 5 inline comments as done. Nebiroth added a comment. Removed test file Added mutex lock when changing CDB Minor code cleanup Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D39571 Files: clangd/ClangdLSPServer.cpp

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. It seems this patch is out of date, we need to merge it with the latests head. Comment at: clangd/DraftStore.cpp:45 std::lock_guard Lock(Mutex); +

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Nebiroth updated this revision to Diff 126449. Nebiroth added a comment. Removed some more empty lines Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D39571 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp clangd/ClangdServer.h

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Nebiroth updated this revision to Diff 126447. Nebiroth added a comment. Herald added a subscriber: klimek. Merged with latest llvm + clang Minor code cleanup Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D39571 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Nebiroth marked 12 inline comments as done. Nebiroth added inline comments. Comment at: test/clangd/did-change-configuration.test:33 +

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:250 + // Verify if path has value and is a valid path + if (Params.settings.compilationDatabasePath.hasValue()) { +CDB.setCompileCommandsDir( Replace `Settings` instead of

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Nebiroth added inline comments. Comment at: clangd/GlobalCompilationDatabase.cpp:108 Logger.log("Failed to find compilation database for " + Twine(File) + - "in overriden directory " + CompileCommandsDir.getValue() + + " in overriden

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Nebiroth updated this revision to Diff 124024. Nebiroth marked 19 inline comments as done. Nebiroth added a comment. Fixed DraftStore thread-safe API being broken Removed superfluous getCompilationDatabase call Changed name of struct to ClangDConfigurationParamsChange Removed operator ! overload

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

malaperle added inline comments. Comment at: clangd/Protocol.h:295 + +struct ClangdConfigurationParams { + ilya-biryukov wrote: > malaperle wrote: > > ilya-biryukov wrote: > > > Maybe call it `ClangdConfigurationParamsChange` to make it clear those > > > are

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/Protocol.h:295 + +struct ClangdConfigurationParams { + malaperle wrote: > ilya-biryukov wrote: > > Maybe call it `ClangdConfigurationParamsChange` to make it clear those are > > diffs, not the actual

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

malaperle added inline comments. Comment at: clangd/Protocol.h:295 + +struct ClangdConfigurationParams { + ilya-biryukov wrote: > Maybe call it `ClangdConfigurationParamsChange` to make it clear those are > diffs, not the actual params? The idea was that we can

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:580 +void ClangdServer::reparseOpenedFiles() { + for (auto Draft : DraftMgr.getDrafts().keys()) { +forceReparse(Draft); Could we have a method in `DraftStore` that returns all active

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Nebiroth updated this revision to Diff 123069. Nebiroth marked 3 inline comments as done. Nebiroth added a comment. Added test for didChangeConfiguration notification. Compilation database and extra file flags are now properly reloaded when changing database path. ClangdConfigurationParams now

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Nebiroth marked 18 inline comments as done. Nebiroth added inline comments. Comment at: clangd/Protocol.h:285 + + DidChangeConfigurationParams() {} + malaperle wrote: > I don't think you need this constructor? I do inside parse() for

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.h:289 + /// ChangedSettings + void changeConfiguration(std::map ChangedSettings); + Nebiroth wrote: > ilya-biryukov wrote: > > This function is way too general for

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Nebiroth added inline comments. Comment at: clangd/ClangdLSPServer.cpp:51 + "definitionProvider": true, + "configurationChangeProvider": true }})"); malaperle wrote: > Are you sure the existing tests don't fail? usually when we change

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. I don't think we should pass the very general configuration map to `ClangdServer`. Especially given that we can easily update `DirectoryBaseCompilationDatabase` instance in `ClangdLSPServer` itself. What are your

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

malaperle requested changes to this revision. malaperle added inline comments. This revision now requires changes to proceed. Comment at: clangd/ClangdLSPServer.cpp:51 + "definitionProvider": true, + "configurationChangeProvider": true }})");

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Nebiroth created this revision. Implementation of DidChangeConfiguration notification handling in clangd. This currently only supports changing one setting: the path of the compilation database to be used for the current project In other words, it is no longer necessary to restart clangd with a