This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][NFC] Add unittests for CallDescription and split the old ones
ClosedPublic

Authored by steakhal on Oct 14 2021, 5:04 AM.

Details

Summary

This NFC change accomplishes three things:

  1. Splits up the single unittest into reasonable segments.
  2. Extends the test infra using a template to select the AST-node from which it is supposed to construct a CallEvent.
  3. Adds a *lot* of different tests, documenting the current capabilities of the CallDescription. The corresponding tests are marked with FIXMEs, where the current behavior should be different.

Both CXXMemberCallExpr and CXXOperatorCallExpr are derived from
CallExpr, so they are matched by using the default template parameter.
On the other hand, CXXConstructExpr is not derived from CallExpr.
In case we want to match for them, we need to pass the type explicitly
to the CallDescriptionAction.

About destructors:
They have no AST-node, but they are generated in the CFG machinery in
the analyzer. Thus, to be able to match against them, we would need to
construct a CFG and walk on that instead of simply walking the AST.

I'm also relaxing the EXPECTation in the
CallDescriptionConsumer::performTest(), to check the LookupResult
only if we matched for the CallDescription.
This is necessary to allow tests in which we expect *no* matches at all.

Diff Detail

Event Timeline

steakhal created this revision.Oct 14 2021, 5:04 AM
steakhal requested review of this revision.Oct 14 2021, 5:04 AM
martong accepted this revision.Oct 14 2021, 7:02 AM

Nice work! LGTM (with some nits)!

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
151

If we had a call expr foo(1, 2) would that match {"foo", None} or {"foo", 2} ?

263

Clever to use this macro!

376

Could you please either have a better and more descriptive name for these tests or could you add a comment? I could hardly found the difference between AliasNames1 and AliasNames2.

Maybe these could go under the same test case with Code1 and Code2 ?

This revision is now accepted and ready to land.Oct 14 2021, 7:02 AM
steakhal updated this revision to Diff 379775.Oct 14 2021, 11:02 AM
steakhal marked 2 inline comments as done.
steakhal marked an inline comment as done.

Restructured the tests for alias handling.

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
151

It actually won't.
The CallDescriptionMap expects at most one CallDescription to match the Call.
Thus, having *two* CDs that we expect to match, breaks this invariant.
Maybe we could have an expensive check enforcing this property, that at most one could match.

In the implementation, we should have the freedom of the evaluation order of the CallDescriptions within the map. It's also important for caching, in the future.

Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2021, 5:58 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript