[PATCH] D72066: [clangd] Assert that the testcases in LocateSymbol.All have no diagnostics

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jan 3 00:26:27 PST 2020


kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

Thanks!



================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:519
+    ASSERT_TRUE(AST.getDiagnostics().empty())
+        << AST.getDiagnostics().begin()->Message << Test;
+
----------------
nit: I would rather keep the loop and just use `ADD_FAILURE()` instead of `llvm::errs()`.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:573
+    // parsing.
+    TU.ExtraArgs.push_back("-fno-delayed-template-parsing");
+
----------------
nit: this one can be dropped.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:577
+    for (auto &Diag : AST.getDiagnostics()) {
+      ASSERT_TRUE(Diag.Severity <= DiagnosticsEngine::Warning)
+          << Diag.Message << Test;
----------------
nit: this looks like an overkill to me, in the end we are trying to test hover behavior, not diagnostics. I would drop the whole loop here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72066/new/

https://reviews.llvm.org/D72066





More information about the cfe-commits mailing list