[PATCH] D103685: [clangd] Drop TestTUs dependency on gtest

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/TestTU.cpp:75
+  if (llvm::sys::fs::createUniqueDirectory("module-cache", ModuleCachePath))
+    llvm_unreachable("Failed to create temp directory for module-cache");
   CI.getHeaderSearchOpts().ModuleCachePath = ModuleCachePath.c_str();
----------------
I don't love using llvm_unreachable for error handling (esp environment sensitive kind like this), it compiles down to UB in ndebug mode.

Log + abort would be preferable I think, though it's one extra line.


================
Comment at: clang-tools-extra/clangd/unittests/TestTU.cpp:119
+    llvm::errs() << "Failed to build code:\n" << Code;
     llvm_unreachable("Failed to build TestTU!");
   }
----------------
I think we should replace this one with abort() too now


================
Comment at: clang-tools-extra/clangd/unittests/TestTU.cpp:122
   if (!AST->getDiagnostics()) {
-    ADD_FAILURE() << "TestTU should always build an AST with a fresh Preamble"
-                  << Code;
+    llvm::errs() << "TestTU should always build an AST with a fresh Preamble"
+                 << Code;
----------------
this check is now a log message that could still let the test pass
Make it an assert instead?

(I think assert without any early return is appropriate per normal llvm style)


================
Comment at: clang-tools-extra/clangd/unittests/TestTU.cpp:148
             << Code;
         break; // Just report first error for simplicity.
       }
----------------
this needs to fail the test. abort()?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103685