[PATCH] D64274: [analyzer] VirtualCallChecker overhaul.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet.
Herald added a project: clang.

This is slightly controversial, please hold me back / bikeshed if you don't like it.

The `optin.cplusplus.VirtualCall` checker in its current shape was implemented by @wangxindsb during his GSoC'2017 project - see D34275 <https://reviews.llvm.org/D34275>. It is a path-sensitive checker that finds calls to virtual functions during construction and destruction. This is a problem because virtual dispatch won't occur during construction or destruction. In particular, if the function is pure virtual, it is outright undefined behavior; in other cases it is simply an unexpected behavior. The checker has an option `PureOnly` to control whether the user is only interested in calls to pure virtual functions. It defaults to false, i.e. warn on non-pure-virtual calls as well.

I propose the following changes:

- Remove the option.
- Instead, split the checker in two:
  - `cplusplus.PureVirtualCall` will only check pure virtual calls and it //will be on by default//.
  - `optin.cplusplus.VirtualCall` will check all calls and will remain opt-in under the old name. It would automatically load `cplusplus.PureVirtualCall`.
- Additionally, remove the bug visitor. The reasons for that have been described in D34275 <https://reviews.llvm.org/D34275>?id=111862#inline-321253: essentially, the visitor doesn't add any new interesting information to the report. It's also currently broken: it places its note in a really weird spot.

I'd like to point out that it breaks backwards compatibility. I'm not going to be hurt by this and i haven't heard a lot about users of this opt-in check but if you know them, please let me know. If we agree to break backwards compatibility, we should make sure that the result is the best, so i'd like to hear @Szelethus's opinion, as he usually has strong opinions on checker options and dependencies :)

See also http://lists.llvm.org/pipermail/cfe-dev/2018-December/060558.html (the thread was bumped recently)


Repository:
  rC Clang

https://reviews.llvm.org/D64274

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/virtualcall.cpp
  clang/test/Analysis/virtualcall.h

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D64274.208248.patch
Type: text/x-patch
Size: 15288 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190706/de63f2da/attachment.bin>