Page MenuHomePhabricator

[analyzer] NFC: Introduce a convenient CallDescriptionMap class.
ClosedPublic

Authored by NoQ on May 24 2019, 7:58 PM.

Details

Summary

"When choosing a container, remember vector is best;
Leave a comment to explain if you choose from the rest!"

-Tony Van Eerd, Postmodern C++.

CallDescriptionMap would encapsulate the procedure of figuring out if any of the supported CallDescriptions apply to a given CallEvent. Without such facility i constantly worry about somebody bashing me on reviews for using slow string switches and stuff. However when a CallDescription isn't a simple string, a linear lookup is almost unavoidable. So even though for now i only implement a linear lookup, i guess with a single interface we can later afford a string-map optimization specifically for simple CallDescriptions.

I also add a few unittests for CallDescription with the help of this class.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.May 24 2019, 7:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2019, 7:58 PM
NoQ retitled this revision from [analyzer] Introduce a convenient CallDescriptionMap class. to [analyzer] NFC: Introduce a convenient CallDescriptionMap class..May 24 2019, 7:59 PM
NoQ updated this revision to Diff 201382.May 24 2019, 8:01 PM

Bring back an assertion in findNode<>().

Szelethus accepted this revision.Jun 4 2019, 5:45 PM

This is why I love unit tests so much, you get the learn a lot about how things work! Thanks!

The idea for the patch is great, can't wait to get rid of the mess we have in MallocChecker.

This revision is now accepted and ready to land.Jun 4 2019, 5:45 PM
xazax.hun requested changes to this revision.Jun 5 2019, 1:55 AM
xazax.hun added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1088 ↗(On Diff #201382)

Will the move constructor work as intended with const members?

1097 ↗(On Diff #201382)

I think in this case copy assignment should also be deleted. Or is it deleted automatically in this case?

This revision now requires changes to proceed.Jun 5 2019, 1:55 AM
NoQ updated this revision to Diff 206951.Jun 27 2019, 3:17 PM

Indeed. Subtle!

NoQ marked 2 inline comments as done.Jun 27 2019, 3:37 PM
Charusso accepted this revision.Jun 28 2019, 5:01 PM

Great patch, thanks you! I wanted to make my own IdentifierInfo array previously.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1060 ↗(On Diff #206951)

What about Optional<>? When I first met that function I have fallen in love.

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
72 ↗(On Diff #206951)

May it worth to mention the second true/false business stands for the call being called.

103 ↗(On Diff #206951)

So {{"bar", "foo"}, true}? I like puzzles, but it would be cool to state out why it is negative as other tests all have negative sub-tests.

NoQ updated this revision to Diff 207403.Jul 1 2019, 1:55 PM
NoQ marked 5 inline comments as done.

Address comments.

Improve the tests so that they failed when not all requested functions were found - it's pretty crude as it only checks that the number of hits is correct, but, i guess, that's what you get when you're trying to write unittests for an intentionally constrained interface.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1060 ↗(On Diff #206951)

I didn't really touch this code, just moved it around, but it's definitely a valid point.

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
103 ↗(On Diff #206951)

{{"bar", "foo"}, false}!

Nice catch tho, typo.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 1 2019, 4:03 PM
Closed by commit rL364866: [analyzer] NFC: Add a convenient CallDescriptionMap class. (authored by dergachev, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 4:03 PM