martong accepted this revision. martong added a comment. Other than Whispy's nits, LGTM!
================ Comment at: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:24 + do \ + if (!LLVM_WITH_Z3) \ + return; \ ---------------- whisperity wrote: > xazax.hun wrote: > > steakhal wrote: > > > xazax.hun wrote: > > > > I think this might not be the idiomatic way to skip a test. Consider > > > > using ` GTEST_SKIP();`. > > > I agree, though that is not yet supported in the `gtest` in the > > > repository. > > > Should we update that to benefit from this macro? > > > > > > There are several places where we could use that like: > > > - > > > [llvm/unittests/ExecutionEngine/MCJIT/MCJITTestAPICommon.h:29](https://github.com/llvm/llvm-project/blob/master/llvm/unittests/ExecutionEngine/MCJIT/MCJITTestAPICommon.h#L29-L33) > > > (expanded 37 times in the codebase) > > > - > > > [llvm/unittests/Support/ThreadPool.cpp:81](https://github.com/llvm/llvm-project/blob/master/llvm/unittests/Support/ThreadPool.cpp#L81-L85) > > > (expanded 7 times in the file) > > > - > > > [llvm/unittests/Support/MemoryTest.cpp:89](https://github.com/llvm/llvm-project/blob/master/llvm/unittests/Support/MemoryTest.cpp#L89-L94) > > > (expanded 10 time in the file) > > I think you should check who updated gtest the last time and ping them what > > is the process to update it again. > Still maybe this could warrant a `// FIXME: Update GTest and use > GTEST_SKIP()` ? +1 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78704/new/ https://reviews.llvm.org/D78704 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
