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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
