xazax.hun added a comment.
You are making a pretty good progress!
I think right now there is some code duplication that could be reduced, but
otherwise, the checker starts to look really good.
================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:43
+private:
+ bool IsVirtualCall(const CallExpr *CE) const;
+
----------------
This could be maybe a free static function instead of a method?
================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:47
+ private:
+ const MemRegion *Region;
+ bool Found;
----------------
Maybe instead of Region the name could be more specific? I.e. the ThisRegion?
Or ObjectRegion?
================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:101
+ DeclName = CD->getNameAsString();
+ InfoText = "Called from this constrctor " + DeclName;
+ } else {
----------------
In the warning messages we usually enclose the source code snippets (e.g.:
declaration names) in single quotes.
================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:135
+ if (isa<CXXDestructorDecl>(MD)) {
+ auto &SVB = C.getSValBuilder();
+ auto ThiSVal =
----------------
getting the SValBuilder is cheap, maybe you could hoist it from the if
statements.
================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:146
+// The EndFunction callback when leave a constructor or a destructor.
+void VirtualCallChecker::checkEndFunction(CheckerContext &C) const {
+ const auto *LCtx = C.getLocationContext();
----------------
The code of this function is almost completely identical with the checkBegin. I
was thinking about abstracting this into a separate function with a bool
argument to determine whether we would like to add something to the GDM or
remove it.
================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:186
-void WalkAST::ReportVirtualCall(const CallExpr *CE, bool isPure) {
- if (ReportPureOnly && !isPure)
+ const auto IC = dyn_cast<CXXInstanceCall>(&Call);
+ if (!IC)
----------------
Maybe you could cast this to CXXMemberCall instead. If that cast succeed, than
you do not need to check the Call.getDecl() and also do not need to check for
ctor/dtor calls.
================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:199
+ // The GDM of constructor and destructor should be true.
+ if (IsVirtualCall(CE) && State->get<CtorMap>(Reg)) {
+ if (IsPureOnly && MD->isPure()) {
----------------
Maybe it would be cleaner to early return if the call is not virtual and remove
it from these two if statements.
================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:213
+ } else {
+ ExplodedNode *N = C.generateNonFatalErrorNode();
+ if (!BT)
----------------
Maybe you could extract the error reporting into a separate function with two
arguments: the error message, and whether it should generate an error node.
https://reviews.llvm.org/D34275
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
