[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Marc-Andre Laperle via Phabricator via cfe-commits]

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