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: llvm-
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
http://lists
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
clangd/ClangdServe
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
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 t
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
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:
clan
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 compile
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 I
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
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 captures
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 che
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 -DMA
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. Server.findDefiniti
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 ge
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 c
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
https://reviews
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:
> > >
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:
> > >
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:
> > >
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:
> > > > ilya-
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:
> > >
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:78
{"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
{"renameProvider", true},
simark wrote:
> ilya-biryukov wrote:
> > si
simark added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:78
{"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
{"renameProvider", true},
ilya-biryukov wrote:
> simark wrote:
> > Nebiroth
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:
> > >
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:78
{"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
{"renameProvider", true},
simark wrote:
> Nebiroth wrote:
> > simark
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
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 FIXM
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
clangd/Clang
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 cod
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 F
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
clangd/Cla
simark added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:78
{"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
{"renameProvider", true},
Nebiroth wrote:
> simark wrote:
> > I find `config
Nebiroth added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:78
{"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
{"renameProvider", true},
simark wrote:
> I find `configurationChangeProvi
simark added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:78
{"documentHighlightProvider", true},
+{"configurationChangeProvider", true},
{"renameProvider", true},
I find `configurationChangeProvider` a bit weird.
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
DidChangeConfigurationP
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
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);
+
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
clangd/
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
c
Nebiroth marked 12 inline comments as done.
Nebiroth added inline comments.
Comment at: test/clangd/did-change-configuration.test:33
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///compile_commands.json","languageId":"json","version":1
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 `Params.s
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 directo
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 f
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 dif
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 params
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
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 fi
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 us
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 DidChangeConfigurationParams
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 `ClangdServer`'s interface,
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 t
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 th
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
}})");
--
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 d
55 matches
Mail list logo
