steakhal marked 4 inline comments as done. steakhal added a comment. I addressed most of the comments.
================ Comment at: clang/unittests/StaticAnalyzer/CheckerRegistration.h:81 +template <AddCheckerFn... Fns> +bool runCheckerOnCodeWithArgs(const std::string &Code, std::string &Diags, + const std::vector<std::string> &Args, ---------------- martong wrote: > Do you use this function template anywhere? No, I'm just following the pattern of the twin-functions above. To be honest, I don't know what is the purpose of the variadic template here. ================ Comment at: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:24 + do \ + if (!LLVM_WITH_Z3) \ + return; \ ---------------- 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) ================ Comment at: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:55 + + bool reachedWithNoContradiction(const CallEvent &, CheckerContext &C, + ProgramStateRef State) const { ---------------- martong wrote: > It this not used in the test? Now I'm using it :) Thx. ================ Comment at: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:65 + + bool reportIfCanBeZero(const CallEvent &Call, CheckerContext &C, + ProgramStateRef State) const { ---------------- martong wrote: > xazax.hun wrote: > > Maybe `reportIfArgCanBeZero`? > Perhaps a generic `reportIfTrue` would be more useful. For that you must use > another eval function instead of `evalEQ` I like it! Fixed. ================ Comment at: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:115 + llvm::raw_string_ostream OS(Diags); + return tooling::runToolOnCodeWithArgs( + std::make_unique<TestAction<addFalsePositiveGenerator>>(OS), Code, Args, ---------------- xazax.hun wrote: > Wasn't `runCheckerOnCodeWithArgs` created for this purpose? Somewhat yes. For testing purposes, it's valuable to encode the testcase's name into the `FileName`, to be visible in the exploded graph enabled for debugging. I have updated the `runCheckerOnCodeWithArgs` function to address your comment. Please note that it introduced a dependency between the corresponding header and the `gtest` header. ================ Comment at: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:138 + std::string Diags2; + EXPECT_TRUE(runFalsePositiveGeneratorOnCode(Code, Diags2)); + EXPECT_EQ(Diags2, "test.FalsePositiveGenerator:REACHED_WITH_CONTRADICTION\n"); ---------------- martong wrote: > There is no need to have `Diags2`. You could reuse `Diags` if in > `runFalsePositiveGeneratorOnCode` you cleared the diag param. I would doubt whether that is more readable. Btw, I moved the declaration closer to the usage. 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
