[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

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


================
Comment at: unittests/clangd/ClangdTests.cpp:94
+
+  bool contains(PathRef P) {
+    std::lock_guard<std::mutex> Lock(Mutex);
----------------
This function should be `const`.
(Would require making `Mutex` mutable, but that's fine)


================
Comment at: unittests/clangd/ClangdTests.cpp:99
+
+  bool lastHadError(PathRef P) {
+    std::lock_guard<std::mutex> Lock(Mutex);
----------------
This function should be const and assert that P is in the map.


================
Comment at: unittests/clangd/ClangdTests.cpp:111
+  std::mutex Mutex;
+  std::map<Path, bool> LastDiagsHadError;
+};
----------------
Maybe replace `std::map<Path,bool>` to `llvm::StringMap<bool>`?


================
Comment at: unittests/clangd/ClangdTests.cpp:492
+
+  EXPECT_TRUE(DiagConsumer.contains(FooCpp));
+  EXPECT_TRUE(DiagConsumer.contains(BarCpp));
----------------
Maybe expose a copy of the map from DiagConsumer and check for all files in a single line?

```
class MultipleErrorCHeckingDiagConsumer {
   /// Exposes all files consumed by onDiagnosticsReady in an unspecified order.
   /// For each file, a bool value indicates whether the last diagnostics contained an error.
   std::vector<std::pair<Path, bool>> filesWithDiags() const { /* ... */ }
};

/// ....
EXPECT_THAT(DiagConsumer.filesWithDiags(), UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true), Pair(BazCpp, false));
```

It would make the test more concise.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571