[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

Fri Apr 24 08:04:28 PDT 2020

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