[PATCH] D68163: [analyzer][MallocChecker][NFC] Change the use of IdentifierInfo* to CallDescription

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked 5 inline comments as done.
Szelethus added inline comments.
Herald added subscribers: martong, steakhal.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:259
   /// calls.
   bool isCalled(const CallDescription &CD) const;
 
----------------
Szelethus wrote:
> NoQ wrote:
> > I don't fully understand how does overload resolution work in this case, maybe rename the original function?
> Well, `isCalled` may now have an arbitrary number of arguments. The single argument version is this function, and the //n//-argument ones call this //n// times: `isCalled(A, B, C)` -> `isCalled(A) || isCalled(B) || isCalled(B)`. I guess I could rename the other one to `isAnyCalled`, but I think its fine.
But that would be super ugly imo :/ I think the current implementation is intuitive enough, but I'm happy to change it if you disagree.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1006
   const FunctionDecl *FD = C.getCalleeDecl(CE);
-  if (!FD)
+  if (!FD || FD->getKind() != Decl::Function)
     return;
----------------
Szelethus wrote:
> kimgr wrote:
> > NoQ wrote:
> > > The `FD->getKind() != Decl::Function` part is super mega redundant here.
> > Sorry for jumping in from nowhere. AFAIK, this is the only way to detect free vs member functions. It looks like this wants to discard member functions. Are you sure it's redundant? 
> Please do! Though I have a suspicion that if this isn't redundant, such a check should be done in `CallDescription`.
I agree that it should be removed, since we explicitly support functions annotated to return a dynamically allocated memory region, //even// if it is a member function.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68163/new/

https://reviews.llvm.org/D68163