sammccall accepted this revision. sammccall added a comment. Awesome, thank you. Just a couple of last nits.
================ Comment at: unittests/clangd/ClangdTests.cpp:984 + std::vector<Diag> Diagnostics) override { + std::lock_guard<std::mutex> Lock(Mutex); + for(const Diag& D : Diagnostics) { ---------------- Locking is unneccesary. (Diagnostics will be delivered once, and the SyncAPI calls block until diagnostics are delivered) ================ Comment at: unittests/clangd/ClangdTests.cpp:1014 + runAddDocument(Server, SourcePath, Test.code()); + EXPECT_TRUE(DiagConsumer.workedAsExpected()); + DiagConsumer.reset(); ---------------- nit: the logic is right, but the message could be better. If we change something and it fails it will print `Expected true: DiagConsumer.worksAsExpected(), but was false` or so. There are a number of things that could be wrong. Prefer just to capture a bit more data (all diagnostics) and use a matcher expression: ``` MATCHER(DeprecationWarning, "") { return arg.Category == "Deprecations" && arg.Severity == DiagnosticsEngine::Warning; } ... EXPECT_THAT(Diags, ElementsAre(DeprecationWarning())); ``` That way if there isn't exactly one diagnostic that's a deprecation warning, it'll print the expectation, the full list of diagnostics, and the reason it didn't match. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51747 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits