[analyzer] Proper caching in CallDescription objects.

[xazax.hun]

Summary

During the review of https://reviews.llvm.org/D29567 it turned out the caching in CallDescription is not implemented properly. In case an identifier does not exist in a translation unit, repeated identifier lookups will be done which might have bad impact on the performance. This patch guarantees that the lookup is only executed once. Moreover this patch fixes a corner case when the identifier of CallDescription does not exist in the translation unit and the called function does not have an identifier (e.g.: overloaded operator in C++).

[NoQ]

Yep, seems that somebody has missed these issues :)

I guess there's no way to test the operator case, because nobody made a CallDescription with an empty name for us (maybe we should even assert that).

[xazax.hun]

In D29884#677387, @NoQ wrote:

Yep, seems that somebody has missed these issues :)

I guess there's no way to test the operator case, because nobody made a CallDescription with an empty name for us (maybe we should even assert that).

Thank you for the review! :) Unfortunately, I did not get where would you put that assert. But in case you think it is important, I would be happy to review a patch :)

[NoQ]

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

73

Maybe assert(FuncName.size() > 0) here?

[xazax.hun]

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

73

Oh, I see now. Yeah, that would make sense :) Do you want me to commit that or will you?

[NoQ]

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

73

Because C++ checkers are receiving a lot of attention recently, i'd probably be looking into modifying this class to work with qualified names and overloads. You can commit though if you want it sooner :)

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

 class CallEvent;
 class CallEventManager;
 
 /// This class represents a description of a function call using the number of
 /// arguments and the name of the function.
 class CallDescription {
   friend CallEvent;
   mutable IdentifierInfo *II;
+  mutable bool IsLookupDone;
   StringRef FuncName;
   unsigned RequiredArgs;
 
 public:
   const static unsigned NoArgRequirement = ~0;
   /// \brief Constructs a CallDescription object.
   ///
   /// @param FuncName The name of the function that will be matched.
   ///
   /// @param RequiredArgs The number of arguments that is expected to match a
   /// call. Omit this parameter to match every occurance of call with a given
   /// name regardless the number of arguments.
   CallDescription(StringRef FuncName, unsigned RequiredArgs = NoArgRequirement)
-      : II(nullptr), FuncName(FuncName), RequiredArgs(RequiredArgs) {}
+      : II(nullptr), IsLookupDone(false), FuncName(FuncName),
+        RequiredArgs(RequiredArgs) {}
 
   /// \brief Get the name of the function that this object matches.
   StringRef getFunctionName() const { return FuncName; }
 };
 
 template<typename T = CallEvent>
 class CallEventRef : public IntrusiveRefCntPtr<const T> {
 public:

cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp

ProgramPoint CallEvent::getProgramPoint(bool IsPreVisit,
   SourceLocation Loc = getSourceRange().getBegin();
   if (IsPreVisit)
     return PreImplicitCall(D, Loc, getLocationContext(), Tag);
   return PostImplicitCall(D, Loc, getLocationContext(), Tag);
 }
 
 bool CallEvent::isCalled(const CallDescription &CD) const {
   assert(getKind() != CE_ObjCMessage && "Obj-C methods are not supported");
-  if (!CD.II)
+  if (!CD.IsLookupDone) {
+    CD.IsLookupDone = true;
     CD.II = &getState()->getStateManager().getContext().Idents.get(CD.FuncName);
-  if (getCalleeIdentifier() != CD.II)
+  }
+  const IdentifierInfo *II = getCalleeIdentifier();
+  if (!II || II != CD.II)
     return false;
   return (CD.RequiredArgs == CallDescription::NoArgRequirement ||
           CD.RequiredArgs == getNumArgs());
 }
 
 SVal CallEvent::getArgSVal(unsigned Index) const {
   const Expr *ArgE = getArgExpr(Index);
   if (!ArgE)