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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet 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();
----------------
sammccall wrote:
> 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.
ah I thought it always compiled down to abort (and sometimes inserted a builtin to indicate noreturn). looks like i've missed the conditional macro definition.


================
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;
----------------
sammccall wrote:
> 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)
aborting instead of just `assert(false)`, since the rest of the code assumes AST->getDiagnostics has a value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103685