xazax.hun added inline comments.
================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:31 +class VirtualCallChecker: public Checker<check::PreCall, check::PostCall> { + mutable std::unique_ptr<BugType> BT_CT; + mutable std::unique_ptr<BugType> BT_DT; ---------------- Could you find more descriptive names for these BugTypes? ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:32 + mutable std::unique_ptr<BugType> BT_CT; + mutable std::unique_ptr<BugType> BT_DT; ---------------- I'd rather have one bug type, describing a call to a virtual function from ctor/dtor. The actual error message can clarify whether this is a call from ctor or from dtor and whether it is pure virtual. You do not need to reflect every detail in the bug type. ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:35 public: - WalkAST(const CheckerBase *checker, BugReporter &br, AnalysisDeclContext *ac, - const CXXMethodDecl *rootMethod, bool isInterprocedural, - bool reportPureOnly) - : Checker(checker), BR(br), AC(ac), RootMethod(rootMethod), - IsInterprocedural(isInterprocedural), ReportPureOnly(reportPureOnly), - visitingCallExpr(nullptr) { - // Walking should always start from either a constructor or a destructor. - assert(isa<CXXConstructorDecl>(rootMethod) || - isa<CXXDestructorDecl>(rootMethod)); - } - - bool hasWork() const { return !WList.empty(); } - - /// This method adds a CallExpr to the worklist and marks the callee as - /// being PreVisited. - void Enqueue(WorkListUnit WLUnit) { - const FunctionDecl *FD = WLUnit->getDirectCallee(); - if (!FD || !FD->getBody()) - return; - Kind &K = VisitedFunctions[FD]; - if (K != NotVisited) - return; - K = PreVisited; - WList.push_back(WLUnit); - } - - /// This method returns an item from the worklist without removing it. - WorkListUnit Dequeue() { - assert(!WList.empty()); - return WList.back(); - } - - void Execute() { - while (hasWork()) { - WorkListUnit WLUnit = Dequeue(); - const FunctionDecl *FD = WLUnit->getDirectCallee(); - assert(FD && FD->getBody()); - - if (VisitedFunctions[FD] == PreVisited) { - // If the callee is PreVisited, walk its body. - // Visit the body. - SaveAndRestore<const CallExpr *> SaveCall(visitingCallExpr, WLUnit); - Visit(FD->getBody()); - - // Mark the function as being PostVisited to indicate we have - // scanned the body. - VisitedFunctions[FD] = PostVisited; - continue; - } + // The flag to determine if pure virtual functions should be issued only + DefaultBool isPureOnly; ---------------- Comments should be full sentences, this means they should be terminated with a period. ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:36 + // The flag to determine if pure virtual functions should be issued only + DefaultBool isPureOnly; ---------------- Variables should start with capital letters. ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:40 + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + bool isCalledbyCtor(const CallExpr *CE,ProgramStateRef state,const LocationContext *LCtx) const; + bool isCalledbyDtor(const CallExpr *CE,ProgramStateRef state,const LocationContext *LCtx) const; ---------------- The names of the arguments should start with an uppercase letter. ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:72 +//GDM (generic data map) to determine if a function is called by an object +REGISTER_TRAIT_WITH_PROGRAMSTATE(ObjectFlag, unsigned) +//GDM (generic data map) to the memregion of this for the ctor and dtor ---------------- Do you need these traits above after having the maps bellow? ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:96 + + if((!CD && !DD) || (Ctorflag!=TrackedCtorDtorFlag && + Dtorflag!=TrackedCtorDtorFlag)) ---------------- The formatting here seems to be off, could you run clang-format on the code? This is a tool that can format the code to comply with the LLVM formatting style guide. ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:129 -void WalkAST::VisitChildren(Stmt *S) { - for (Stmt *Child : S->children()) - if (Child) - Visit(Child); -} + const auto *CC = dyn_cast_or_null<CXXConstructorCall>(&Call); + const auto *CD = dyn_cast_or_null<CXXDestructorCall>(&Call); ---------------- I think you do not need the or_null suffix here since the argument of this cast will never be null. ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:141 + // Enter a constructor, increase the corresponding integer + if (dyn_cast<CXXConstructorDecl>(D)) { + unsigned Constructorflag = State->get<ConstructorFlag>(); ---------------- If you do not use the result of `dyn_cast`, you could use `isa` instead. Even better, you could reuse `CC` or `DC` in this check. ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:168 // this as a non-virtual call and do not warn. - if (CME->getQualifier()) - callIsNonVirtual = true; + if (Expr *Base = CME->getBase()->IgnoreImpCasts()) { + if (!isa<CXXThisExpr>(Base)) { ---------------- Shouldn't this be const? ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:192 + } + else { + BT_CT.reset(new BugType(this, ---------------- In LLVM we do not write the braces for conditionals that guarding a single statement. ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:218 + } + ExplodedNode *N = C.generateNonFatalErrorNode(); + auto Reporter = llvm::make_unique<BugReport>(*BT_DT, BT_DT->getName(), N); ---------------- In case of calling a pure virtual function maybe it would be better to stop the analysis, i.e.: creating a fatal error node. Also, these functions might fail to give you back an ExplodedNode, so I think you should have a check here for that case. ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:314 + return None; + const CXXMethodDecl *MD = dyn_cast_or_null<CXXMethodDecl>(FD->getParent()); + if (!MD) ---------------- Can getParent ever return null? Maybe you do not need the or_null suffix in the cast. Moreover are you sure that this method works? In case the function was a method, the parent should be the CXXRecordDecl, not a CXXMethodDecl. ================ Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:332 +// Check the base of the callexpr is equal to the this of the ctor +bool VirtualCallChecker::isCalledbyCtor(const CallExpr *CE,ProgramStateRef State,const LocationContext *LCtx) const { + if (const MemberExpr *CME = dyn_cast<MemberExpr>(CE->getCallee())) { ---------------- Maybe instead of callExpr, you should get CXXInstanceCall, which has a getCXXThisVal method. https://reviews.llvm.org/D34275 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits