<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">LGTM, I have couple of comments below.<div><br></div><div>- Fariborz</div><div><br><div><div>On Jul 25, 2012, at 5:27 PM, Anna Zaks wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Author: zaks<br>Date: Wed Jul 25 19:27:51 2012<br>New Revision: 160768<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=160768&view=rev">http://llvm.org/viewvc/llvm-project?rev=160768&view=rev</a><br>Log:<br>[analyzer] Inline ObjC class methods.<br><br>- Some cleanup(the TODOs) will be done after ObjC method inlining is<br>complete.<br>- Simplified CallEvent::getDefinition not to require ISDynamicDispatch<br>parameter.<br>- Also addressed Jordan's comments from r160530.<br><br>Added:<br> cfe/trunk/test/Analysis/inlining/<br> cfe/trunk/test/Analysis/inlining/InlineObjCClassMethod.m<br>Modified:<br> cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Calls.h<br> cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h<br> cfe/trunk/lib/StaticAnalyzer/Core/Calls.cpp<br> cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp<br> cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp<br><br>Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Calls.h<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Calls.h?rev=160768&r1=160767&r2=160768&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Calls.h?rev=160768&r1=160767&r2=160768&view=diff</a><br>==============================================================================<br>--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Calls.h (original)<br>+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Calls.h Wed Jul 25 19:27:51 2012<br>@@ -121,15 +121,9 @@<br> const Decl *getDecl() const;<br><br> /// \brief Returns the definition of the function or method that will be<br>- /// called. May be null.<br>- ///<br>- /// This is used when deciding how to inline the call.<br>- ///<br>- /// \param IsDynamicDispatch True if the definition returned may not be the<br>- /// definition that is actually invoked at runtime. Note that if we have<br>- /// sufficient type information to devirtualize a dynamic method call,<br>- /// we will (and \p IsDynamicDispatch will be set to \c false).<br>- const Decl *getDefinition(bool &IsDynamicDispatch) const;<br>+ /// called. Returns NULL if the definition cannot be found; ex: due to<br>+ /// dynamic dispatch in ObjC methods.<br>+ const Decl *getRuntimeDefinition() const;<br><br> /// \brief Returns the expression whose value will be the result of this call.<br> /// May be null.<br>@@ -289,8 +283,7 @@<br> return cast_or_null<FunctionDecl>(CallEvent::getDecl());<br> }<br><br>- const Decl *getDefinition(bool &IsDynamicDispatch) const {<br>- IsDynamicDispatch = false;<br>+ const Decl *getRuntimeDefinition() const {<br> const FunctionDecl *FD = getDecl();<br> // Note that hasBody() will fill FD with the definition FunctionDecl.<br> if (FD && FD->hasBody(FD))<br>@@ -371,7 +364,7 @@<br> : SimpleCall(CE, St, LCtx, K) {}<br><br> public:<br>- const Decl *getDefinition(bool &IsDynamicDispatch) const;<br>+ const Decl *getRuntimeDefinition() const;<br><br> static bool classof(const CallEvent *CA) {<br> return CA->getKind() >= CE_BEG_CXX_INSTANCE_CALLS &&<br>@@ -457,8 +450,7 @@<br> return BR->getDecl();<br> }<br><br>- const Decl *getDefinition(bool &IsDynamicDispatch) const {<br>- IsDynamicDispatch = false;<br>+ const Decl *getRuntimeDefinition() const {<br> return getBlockDecl();<br> }<br><br>@@ -551,7 +543,7 @@<br> unsigned getNumArgs() const { return 0; }<br><br> SVal getCXXThisVal() const;<br>- const Decl *getDefinition(bool &IsDynamicDispatch) const;<br>+ const Decl *getRuntimeDefinition() const;<br><br> static bool classof(const CallEvent *CA) {<br> return CA->getKind() == CE_CXXDestructor;<br>@@ -620,6 +612,8 @@<br> void getExtraInvalidatedRegions(RegionList &Regions) const;<br><br> QualType getDeclaredResultType() const;<br>+ ObjCMethodDecl *LookupClassMethodDefinition(Selector Sel,<br>+ ObjCInterfaceDecl *ClassDecl) const;<br><br> public:<br> ObjCMethodCall(const ObjCMessageExpr *Msg, ProgramStateRef St,<br>@@ -678,18 +672,23 @@<br> llvm_unreachable("Unknown message kind");<br> }<br><br>- const Decl *getDefinition(bool &IsDynamicDispatch) const {<br>- IsDynamicDispatch = true;<br>- <br>- const ObjCMethodDecl *MD = getDecl();<br>- if (!MD)<br>- return 0;<br>+ // TODO: We might want to only compute this once (or change the API for <br>+ // getting the parameters). Currently, this gets called 3 times during <br>+ // inlining.<br>+ const Decl *getRuntimeDefinition() const {<br><br>- for (Decl::redecl_iterator I = MD->redecls_begin(), E = MD->redecls_end();<br>- I != E; ++I) {<br>- if (cast<ObjCMethodDecl>(*I)->isThisDeclarationADefinition())<br>- return *I;<br>+ const ObjCMessageExpr *E = getOriginExpr();<br></div></blockquote><div><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; ">getOriginExpr may return null. You should check for that.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "><br></div><blockquote type="cite"><div>+ if (E->isInstanceMessage()) {<br>+ return 0;<br>+ } else {<br>+ // This is a calss method.<br>+ // If we have type info for the receiver class, we are calling via<br>+ // class name.<br>+ if (ObjCInterfaceDecl *IDecl = E->getReceiverInterface()) {<br>+ return LookupClassMethodDefinition(E->getSelector(), IDecl);<br>+ }<br> }<br>+<br> return 0;<br> }<br><br>@@ -770,8 +769,8 @@<br> DISPATCH(getDecl);<br> }<br><br>-inline const Decl *CallEvent::getDefinition(bool &IsDynamicDispatch) const {<br>- DISPATCH_ARG(getDefinition, IsDynamicDispatch);<br>+inline const Decl *CallEvent::getRuntimeDefinition() const {<br>+ DISPATCH(getRuntimeDefinition);<br> }<br><br> inline unsigned CallEvent::getNumArgs() const {<br><br>Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h?rev=160768&r1=160767&r2=160768&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h?rev=160768&r1=160767&r2=160768&view=diff</a><br>==============================================================================<br>--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h (original)<br>+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h Wed Jul 25 19:27:51 2012<br>@@ -471,7 +471,7 @@<br> void evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred,<br> const SimpleCall &Call);<br><br>- /// \bried Default implementation of call evaluation.<br>+ /// \brief Default implementation of call evaluation.<br> void defaultEvalCall(NodeBuilder &B, ExplodedNode *Pred,<br> const CallEvent &Call);<br> private:<br><br>Modified: cfe/trunk/lib/StaticAnalyzer/Core/Calls.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/Calls.cpp?rev=160768&r1=160767&r2=160768&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/Calls.cpp?rev=160768&r1=160767&r2=160768&view=diff</a><br>==============================================================================<br>--- cfe/trunk/lib/StaticAnalyzer/Core/Calls.cpp (original)<br>+++ cfe/trunk/lib/StaticAnalyzer/Core/Calls.cpp Wed Jul 25 19:27:51 2012<br>@@ -201,8 +201,7 @@<br><br> CallEvent::param_iterator<br> AnyFunctionCall::param_begin(bool UseDefinitionParams) const {<br>- bool IgnoredDynamicDispatch;<br>- const Decl *D = UseDefinitionParams ? getDefinition(IgnoredDynamicDispatch)<br>+ const Decl *D = UseDefinitionParams ? getRuntimeDefinition()<br> : getDecl();<br> if (!D)<br> return 0;<br>@@ -212,8 +211,7 @@<br><br> CallEvent::param_iterator<br> AnyFunctionCall::param_end(bool UseDefinitionParams) const {<br>- bool IgnoredDynamicDispatch;<br>- const Decl *D = UseDefinitionParams ? getDefinition(IgnoredDynamicDispatch)<br>+ const Decl *D = UseDefinitionParams ? getRuntimeDefinition()<br> : getDecl();<br> if (!D)<br> return 0;<br>@@ -354,8 +352,8 @@<br> }<br><br><br>-const Decl *CXXInstanceCall::getDefinition(bool &IsDynamicDispatch) const {<br>- const Decl *D = SimpleCall::getDefinition(IsDynamicDispatch);<br>+const Decl *CXXInstanceCall::getRuntimeDefinition() const {<br>+ const Decl *D = SimpleCall::getRuntimeDefinition();<br> if (!D)<br> return 0;<br><br>@@ -368,8 +366,7 @@<br> if (const CXXMethodDecl *Devirtualized = devirtualize(MD, getCXXThisVal()))<br> return Devirtualized;<br><br>- IsDynamicDispatch = true;<br>- return MD;<br>+ return 0;<br> }<br><br><br>@@ -458,8 +455,8 @@<br> Regions.push_back(static_cast<const MemRegion *>(Data));<br> }<br><br>-const Decl *CXXDestructorCall::getDefinition(bool &IsDynamicDispatch) const {<br>- const Decl *D = AnyFunctionCall::getDefinition(IsDynamicDispatch);<br>+const Decl *CXXDestructorCall::getRuntimeDefinition() const {<br>+ const Decl *D = AnyFunctionCall::getRuntimeDefinition();<br> if (!D)<br> return 0;<br><br>@@ -472,15 +469,13 @@<br> if (const CXXMethodDecl *Devirtualized = devirtualize(MD, getCXXThisVal()))<br> return Devirtualized;<br><br>- IsDynamicDispatch = true;<br>- return MD;<br>+ return 0;<br> }<br><br><br> CallEvent::param_iterator<br> ObjCMethodCall::param_begin(bool UseDefinitionParams) const {<br>- bool IgnoredDynamicDispatch;<br>- const Decl *D = UseDefinitionParams ? getDefinition(IgnoredDynamicDispatch)<br>+ const Decl *D = UseDefinitionParams ? getRuntimeDefinition()<br> : getDecl();<br> if (!D)<br> return 0;<br>@@ -490,8 +485,7 @@<br><br> CallEvent::param_iterator<br> ObjCMethodCall::param_end(bool UseDefinitionParams) const {<br>- bool IgnoredDynamicDispatch;<br>- const Decl *D = UseDefinitionParams ? getDefinition(IgnoredDynamicDispatch)<br>+ const Decl *D = UseDefinitionParams ? getRuntimeDefinition()<br> : getDecl();<br> if (!D)<br> return 0;<br>@@ -593,3 +587,34 @@<br> return OCM_Message;<br> return static_cast<ObjCMessageKind>(Info.getInt());<br> }<br>+<br>+// TODO: This implementation is copied from SemaExprObjC.cpp, needs to be<br>+// factored into the ObjCInterfaceDecl.<br>+ObjCMethodDecl *ObjCMethodCall::LookupClassMethodDefinition(Selector Sel,<br>+ ObjCInterfaceDecl *ClassDecl) const {<br>+ ObjCMethodDecl *Method = 0;<br>+ // Lookup in class and all superclasses.<br>+ while (ClassDecl && !Method) {<br>+ if (ObjCImplementationDecl *ImpDecl = ClassDecl->getImplementation())<br>+ Method = ImpDecl->getClassMethod(Sel);<br>+<br>+ // Look through local category implementations associated with the class.<br>+ if (!Method)<br>+ Method = ClassDecl->getCategoryClassMethod(Sel);<br>+<br>+ // Before we give up, check if the selector is an instance method.<br>+ // But only in the root. This matches gcc's behavior and what the<br>+ // runtime expects.<br>+ if (!Method && !ClassDecl->getSuperClass()) {<br>+ Method = ClassDecl->lookupInstanceMethod(Sel);<br></div></blockquote><div><br></div>You don't want this, do you?</div><div><br></div><div>- Fariborz</div><div><br></div><br></div></body></html>