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