Page MenuHomePhabricator

[analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor
ClosedPublic

Authored by steakhal on Apr 23 2020, 4:53 AM.

Details

Summary

Adds the test infrastructure for testing the FalsePositiveRefutationBRVisitor.

It will be extended in the D78457 patch, which demonstrates and fixes a bug in
the visitor.

Diff Detail

Event Timeline

steakhal created this revision.Apr 23 2020, 4:53 AM
steakhal updated this revision to Diff 259531.Apr 23 2020, 5:09 AM

Upload the right diff.

Thanks! Having more tests is always welcome!

I mentioned some nits inline, but I wonder if you actually need to add a new check. Can't you just reuse existing debug checks?
We have the expr inspeciton checker that supports the following functions:

clang_analyzer_warnIfReached
clang_analyzer_eval
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
25

I think this might not be the idiomatic way to skip a test. Consider using GTEST_SKIP();.

66

Maybe reportIfArgCanBeZero?

116

Wasn't runCheckerOnCodeWithArgs created for this purpose?

I like it! Nice work! I have some minor comments.

clang/unittests/StaticAnalyzer/CheckerRegistration.h
81

Do you use this function template anywhere?

clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
139

There is no need to have Diags2. You could reuse Diags if in runFalsePositiveGeneratorOnCode you cleared the diag param.

martong added inline comments.Apr 23 2020, 7:20 AM
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
56

It this not used in the test?

martong added inline comments.Apr 23 2020, 7:24 AM
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
66

Perhaps a generic reportIfTrue would be more useful. For that you must use another eval function instead of evalEQ

steakhal updated this revision to Diff 259668.Apr 23 2020, 12:15 PM
steakhal marked 6 inline comments as done.
steakhal marked 4 inline comments as done.Apr 23 2020, 12:21 PM

I addressed most of the comments.

clang/unittests/StaticAnalyzer/CheckerRegistration.h
81

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.

clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
25

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:

56

Now I'm using it :) Thx.

66

I like it! Fixed.

116

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.

139

I would doubt whether that is more readable.
Btw, I moved the declaration closer to the usage.

xazax.hun added inline comments.Apr 23 2020, 2:15 PM
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
25

I think you should check who updated gtest the last time and ping them what is the process to update it again.

xazax.hun accepted this revision.Apr 23 2020, 2:16 PM

I think this should not be blocked on the gtest update. If getting an updated gtest into the repo takes to much time and the reviewers are happy, I'm fine with doing that change as a follow-up.

Overall it looks good to me but wait for some more reviews before committing.

This revision is now accepted and ready to land.Apr 23 2020, 2:16 PM
whisperity added inline comments.Apr 24 2020, 2:46 AM
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
25

Still maybe this could warrant a // FIXME: Update GTest and use GTEST_SKIP() ?

martong accepted this revision.Apr 24 2020, 7:35 AM

Other than Whispy's nits, LGTM!

clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
25

+1

steakhal marked 6 inline comments as done.Apr 25 2020, 8:26 AM
steakhal updated this revision to Diff 260104.Apr 25 2020, 8:35 AM
steakhal edited the summary of this revision. (Show Details)
  • added GTEST_SKIP FIXME comment
  • reformatted the summary of the patch

I reverted your commit because it seemed to have broken the build:

FalsePositiveRefutationBRVisitorTest.cpp:112:3: error: use of undeclared identifier 'LLVM_WITH_Z3'

https://github.com/llvm/llvm-project/commit/a44425f25b5ca417e7ecee6e7e00040224e50a69

This revision was automatically updated to reflect the committed changes.

This commit breaks standalone clang builds.

clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
20

config.h is a private LLVM header and must not be used from clang.

steakhal added inline comments.Jul 29 2020, 4:23 AM
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
20

I'm not a CMake profession, but shouldn't it be declared private to the LLVM target in the corresponding CMakeLists file then?

How do you query whether clang has built with Z3 or not @mgorny?
I'm including that header only for that.

whisperity added inline comments.Jul 29 2020, 6:02 AM
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
20

I've did a quick skim of the code, and it seems there is no way in it currently to query this.
Your best bet would be either adding an extra flag to Clang's CMake generated Config header that inherits this flag, or checking llvm::CreateZ3Solver() - right now, this method reports a fatal error, but you could create a bool function in the header which reports constant true or false, and turn the test's skip into a runtime condition?

steakhal added inline comments.Jul 29 2020, 6:58 AM
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
20

How could I use a private header in the first place?

steakhal added inline comments.Jul 30 2020, 3:34 AM
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
20

The fix is on the way: D84929