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
}})");
----------------
Are you sure the existing tests don't fail? usually when we change this, some
tests have to be adjusted.
================
Comment at: clangd/ClangdLSPServer.cpp:199
+ Ctx C, DidChangeConfigurationParams &Params) {
+ std::map<std::string, std::string> SettingsMap;
+ SettingsMap.insert(std::pair<std::string, std::string>(
----------------
I'm thinking, maybe it's better not to have the map and just pass along the
ClangdConfigurationParams to the server (instead of the map). I think
ClangdConfigurationParams is more useful as a structure than a "flat" map with
all the keys being at the same level. In ClangdConfigurationParams, we'll be
able to add sub-configurations sections (index, code-completion, more?) which
is well suited to reflect the JSON format.
Unless perhaps you had another use case for the map that I'm not thinking about?
================
Comment at: clangd/ClangdLSPServer.cpp:201
+ SettingsMap.insert(std::pair<std::string, std::string>(
+ "CDBPath", Params.settings.compilationDatabasePath.getValue()));
+
CDB.setCompileCommandsDir(Params.settings.compilationDatabasePath.getValue());
----------------
Won't getValue crash if compilationDatabasePath was not set since it's optional?
================
Comment at: clangd/ClangdLSPServer.cpp:203
+
CDB.setCompileCommandsDir(Params.settings.compilationDatabasePath.getValue());
+
CDB.getCompilationDatabase(StringRef(CDB.getCompileCommandsDir().getValue()));
+
----------------
Why is this line needed? Maybe there should be a comment?
I think its meant to reload the actual CompilationDatabase object? I think you
could call this in setCompileCommandsDir and not have to make
getCompilationDatabase public. You also woudn't need getCompileCommandsDir at
all.
================
Comment at: clangd/ClangdLSPServer.cpp:208
+ // map and sent with this function call.
+ Server.changeConfiguration(SettingsMap);
+}
----------------
Pass Params.settings? see previous comment.
================
Comment at: clangd/ClangdServer.cpp:441
+ std::map<std::string, std::string> ChangedSettings) {
+ if (!ChangedSettings.empty()) {
+ }
----------------
I think we need to discuss what should happen when the compilation database
changes. Since this is mainly for switching "configuration", I think it should
invalidate all previously parsed units. Otherwise, if the user has a file open
in the editor, it will report diagnostics based on the old configuration and
similarly code completion will be outdated (until Clangd is restarted?). Would
it be unreasonable to reparse all units in memory? Perhaps
ClangdServer::forceReparse would be useful for that?
I'm not sure what is the right approach but we should make sure the editors are
not left in a confusing state.
================
Comment at: clangd/GlobalCompilationDatabase.h:55
getCompileCommands(PathRef File) override;
+ llvm::Optional<Path> getCompileCommandsDir() { return CompileCommandsDir; }
+ void setCompileCommandsDir(Path P) { CompileCommandsDir = P; }
----------------
remove?
================
Comment at: clangd/GlobalCompilationDatabase.h:56
+ llvm::Optional<Path> getCompileCommandsDir() { return CompileCommandsDir; }
+ void setCompileCommandsDir(Path P) { CompileCommandsDir = P; }
+
----------------
call getCompilationDatabase from here?
================
Comment at: clangd/GlobalCompilationDatabase.h:58
+
+ tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
----------------
move back to private?
================
Comment at: clangd/Protocol.cpp:534
+llvm::Optional<DidChangeConfigurationParams>
+DidChangeConfigurationParams::parse(llvm::yaml::MappingNode *Params,
----------------
I think this needs a test, perhaps create did-change-configuration.test? It can
also test the ClangdConfigurationParams::parse code
If you do, I think it's a good idea to test a few failing cases:
- "settings" is not a mapping node. You can test this with a scalar node like
"settings": ""
- Something else than "settings" is present, so that it goes through
logIgnoredField
- "settings" is not set at all. In this case, because it's mandatory in the
protocol, return llvm::None. This can be checked after the loop after all
key/values were read.
There are similar tests in execute-command.test if you'd like an example.
And of course also add a "successful" case as well :)
================
Comment at: clangd/Protocol.cpp:561
+
+ return Result;
+}
----------------
If "settings" was not set in the end, return llvm::None, because this is
mandatory in the protocol.
================
Comment at: clangd/Protocol.cpp:578
+ if (!Value)
+ return llvm::None;
+
----------------
Here there should be a test when "compilationDatabasePath" is not a scalar
node. You can test this with a scalar node like "compilationDatabasePath": {}
================
Comment at: clangd/Protocol.cpp:582
+ Result.compilationDatabasePath = Value->getValue(KeyStorage);
+ }
+ }
----------------
else {
logIgnoredField(KeyValue, Logger);
}
?
Plus in the test this should be easy to cover.
settings {
"customField": ""
}
================
Comment at: clangd/Protocol.h:276
+struct ClangdConfigurationParams {
+
----------------
I think there should to be comment to mention that this is a Clangd extension.
================
Comment at: clangd/Protocol.h:285
+
+ DidChangeConfigurationParams() {}
+
----------------
I don't think you need this constructor?
================
Comment at: clangd/Protocol.h:287
+
+ DidChangeConfigurationParams(ClangdConfigurationParams settings)
+ : settings(settings) {}
----------------
Maybe remove this to make it more consistent with the other interfaces/struct.
We usually just assign to fields after the struct is instantiated, not in the
constructor.
================
Comment at: clangd/Protocol.h:290
+
+ ClangdConfigurationParams settings;
+
----------------
I think there should to be comment to mention that this is a Clangd extension.
Here, it would also explain that the protocol specifies "any" and that using a
predefined ClangdConfigurationParams struct is both easier to use and safer.
https://reviews.llvm.org/D39571
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
