Page MenuHomePhabricator

[clang-tidy] Add calling virtual functions in constructors/destructors check.
Needs ReviewPublic

Authored by hokein on Jan 8 2016, 12:36 PM.

Details

Diff Detail

Event Timeline

hokein updated this revision to Diff 44362.Jan 8 2016, 12:36 PM
hokein retitled this revision from to [clang-tidy] Add calling virtual functions in constructors/destructors check..
hokein updated this object.
hokein set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: Eugene.Zelenko.EditedJan 8 2016, 5:25 PM

This check is duplicate of clang-analyzer-alpha.cplusplus.VirtualCall.

From my point of view, Clang-tidy is better place, since such calls doesn't depend of run-time paths.

I think will be good idea to try to establish better functionality separation between Clang/Clang-tidy/Clang Static Analyzer. Current situation looks like different teams try to take everything coming to them without considering big picture. This my impression based on including padding check in Clang Static Analyzer.

I may be wrong, but Clang seems even better place for such warnings.

This check is duplicate of clang-analyzer-alpha.cplusplus.VirtualCall.

Oops... Didn't notice there is an implementation already.

From my point of view, Clang-tidy is better place, since such calls doesn't depend of run-time paths.

I think will be good idea to try to establish better functionality separation between Clang/Clang-tidy/Clang Static Analyzer. Current situation looks like different teams try to take everything coming to them without considering big picture. This my impression based on including padding check in Clang Static Analyzer.

I may be wrong, but Clang seems even better place for such warnings.

However clang-tidy has a ' clang-analyzer-*' check option to run the clang static analyzer check. I'm not sure whether it's worthwhile to move VirtualCall check to the module in clang-tidy.

See D14779 for discussion about Clang-tidy vs Static Analyzer.

Any further thoughts here?
I was slightly bitten by this recently, and i though that it already existed as a clang-tidy check (:

If the CSA checker is still in alpha, I'd proceed with this check instead of investing time in polishing the CSA implementation.

Aaron, WDYT?

If the CSA checker is still in alpha, I'd proceed with this check instead of investing time in polishing the CSA implementation.

Do you know why the CSA checker is still in alpha? As best I can tell, that check and this one are attempting to do the same work. I'm kind of worried about having competing functionalities because users have to guess as to which one is the "correct" one to use.

Do you know why the CSA checker is still in alpha?

It isn't - https://reviews.llvm.org/D26768 moved it to optin.

I don't know why https://reviews.llvm.org/D34275 didn't turn it on by default.

Do you know why the CSA checker is still in alpha?

It isn't - https://reviews.llvm.org/D26768 moved it to optin.

I don't know why https://reviews.llvm.org/D34275 didn't turn it on by default.

Then I'm not certain the utility of this check given that you can call the CSA check from clang-tidy directly; however, I do like the idea of exposing the CSA check under the name cert-oop50-cpp.