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