[analyzer][NFC] Switch to using CallDescription::matches() instead of isCalled()

[steakhal]

Summary

This patch replaces each use of the previous API with the new one.
In variadic cases, it will use the ADL matchesAny(Call, CDs...) variadic function.
Also simplifies some code involving such operations.

[steakhal]

use the terser ADL syntax matches_any(Call, CD1, CDs...)

[xazax.hun]

Once I understand the motivation behind CallDescription::matches(), this patch looks good. The use of variadic matching is a nice cleanup.

[martong]

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2275

I think this would be more unified to the rest and would make further refactoring easier (considering my other comment https://reviews.llvm.org/D113590#inline-1089657)

[steakhal]

rebase

[steakhal]

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2275

I think the member function is more readable.
I could reject single argument calls of matches_any() to have consistency across single CallDescription checking.
Or do the opposite, basically what you propose. Although, I would still stick to the member function syntax. It's probably a personal preference.

What I tried to do is to have the exact same name for both. That would play nicely with the Stroustrup's unified member function call syntax (brevzin.github.io blog post).
However, I could not make it work :/

[martong]

LGTM!

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2275

Okay, let's not get stuck on nuances.

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

bool isInSystemHeader() const {
     // Special case for implicitly-declared global operator new/delete.
     // These should be considered system functions.
     if (const auto *FD = dyn_cast<FunctionDecl>(D))
       return FD->isOverloadedOperator() && FD->isImplicit() && FD->isGlobal();
 
     return false;
   }
 
-  /// Returns true if the CallEvent is a call to a function that matches
-  /// the CallDescription.
-  ///
-  /// Note that this function is not intended to be used to match Obj-C method
-  /// calls.
-  bool isCalled(const CallDescription &CD) const;
-
-  /// Returns true whether the CallEvent is any of the CallDescriptions supplied
-  /// as a parameter.
-  template <typename FirstCallDesc, typename... CallDescs>
-  bool isCalled(const FirstCallDesc &First, const CallDescs &... Rest) const {
-    return isCalled(First) || isCalled(Rest...);
-  }
-
   /// Returns a source range for the entire call, suitable for
   /// outputting in diagnostics.
   virtual SourceRange getSourceRange() const {
     return getOriginExpr()->getSourceRange();
   }
 
   /// Returns the value of a given argument at the time of the call.
   virtual SVal getArgSVal(unsigned Index) const;

clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

 
 void CFRetainReleaseChecker::checkPreCall(const CallEvent &Call,
                                           CheckerContext &C) const {
   // TODO: Make this check part of CallDescription.
   if (!Call.isGlobalCFunction())
     return;
 
   // Check if we called CFRetain/CFRelease/CFMakeCollectable/CFAutorelease.
-  if (!(Call.isCalled(CFRetain) || Call.isCalled(CFRelease) ||
-        Call.isCalled(CFMakeCollectable) || Call.isCalled(CFAutorelease)))
+
+  if (!matchesAny(Call, CFRetain, CFRelease, CFMakeCollectable, CFAutorelease))
     return;
 
   // Get the argument's value.
   SVal ArgVal = Call.getArgSVal(0);
   Optional<DefinedSVal> DefArgVal = ArgVal.getAs<DefinedSVal>();
   if (!DefArgVal)
     return;
 

clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

if (!IdentifierInfoInitialized) {
      * only once. */
     IILockGuard  = &Ctx.Idents.get(ClassLockGuard);
     IIUniqueLock = &Ctx.Idents.get(ClassUniqueLock);
     IdentifierInfoInitialized = true;
   }
 }
 
 bool BlockInCriticalSectionChecker::isBlockingFunction(const CallEvent &Call) const {
-  if (Call.isCalled(SleepFn)
-      || Call.isCalled(GetcFn)
-      || Call.isCalled(FgetsFn)
-      || Call.isCalled(ReadFn)
-      || Call.isCalled(RecvFn)) {
-    return true;
-  }
-  return false;
+  return matchesAny(Call, SleepFn, GetcFn, FgetsFn, ReadFn, RecvFn);
 }
 
 bool BlockInCriticalSectionChecker::isLockFunction(const CallEvent &Call) const {
   if (const auto *Ctor = dyn_cast<CXXConstructorCall>(&Call)) {
     auto IdentifierInfo = Ctor->getDecl()->getParent()->getIdentifier();
     if (IdentifierInfo == IILockGuard || IdentifierInfo == IIUniqueLock)
       return true;
   }
 
-  if (Call.isCalled(LockFn)
-      || Call.isCalled(PthreadLockFn)
-      || Call.isCalled(PthreadTryLockFn)
-      || Call.isCalled(MtxLock)
-      || Call.isCalled(MtxTimedLock)
-      || Call.isCalled(MtxTryLock)) {
-    return true;
-  }
-  return false;
+  return matchesAny(Call, LockFn, PthreadLockFn, PthreadTryLockFn, MtxLock,
+                    MtxTimedLock, MtxTryLock);
 }
 
 bool BlockInCriticalSectionChecker::isUnlockFunction(const CallEvent &Call) const {
   if (const auto *Dtor = dyn_cast<CXXDestructorCall>(&Call)) {
     const auto *DRecordDecl = cast<CXXRecordDecl>(Dtor->getDecl()->getParent());
     auto IdentifierInfo = DRecordDecl->getIdentifier();
     if (IdentifierInfo == IILockGuard || IdentifierInfo == IIUniqueLock)
       return true;
   }
 
-  if (Call.isCalled(UnlockFn)
-       || Call.isCalled(PthreadUnlockFn)
-       || Call.isCalled(MtxUnlock)) {
-    return true;
-  }
-  return false;
+  return matchesAny(Call, UnlockFn, PthreadUnlockFn, MtxUnlock);
 }
 
 void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call,
                                                   CheckerContext &C) const {
   initIdentifierInfo(C.getASTContext());
 
   if (!isBlockingFunction(Call)
       && !isLockFunction(Call)

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

CStringChecker::FnCheck CStringChecker::identifyCall(const CallEvent &Call,
   const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
   if (!CE)
     return nullptr;
 
   const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
   if (!FD)
     return nullptr;
 
-  if (Call.isCalled(StdCopy)) {
+  if (StdCopy.matches(Call))
     return &CStringChecker::evalStdCopy;
-  } else if (Call.isCalled(StdCopyBackward)) {
+  if (StdCopyBackward.matches(Call))
     return &CStringChecker::evalStdCopyBackward;
-  }
 
   // Pro-actively check that argument types are safe to do arithmetic upon.
   // We do not want to crash if someone accidentally passes a structure
   // into, say, a C++ overload of any of these functions. We could not check
   // that for std::copy because they may have arguments of other types.
   for (auto I : CE->arguments()) {
     QualType T = I->getType();
     if (!T->isIntegralOrEnumerationType() && !T->isPointerType())

clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp

 private:
   void evalChroot(const CallEvent &Call, CheckerContext &C) const;
   void evalChdir(const CallEvent &Call, CheckerContext &C) const;
 };
 
 } // end anonymous namespace
 
 bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
-  if (Call.isCalled(Chroot)) {
+  if (Chroot.matches(Call)) {
     evalChroot(Call, C);
     return true;
   }
-  if (Call.isCalled(Chdir)) {
+  if (Chdir.matches(Call)) {
     evalChdir(Call, C);
     return true;
   }
 
   return false;
 }
 
 void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
 
   C.addTransition(state);
 }
 
 // Check the jail state before any function call except chroot and chdir().
 void ChrootChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
   // Ignore chroot and chdir.
-  if (Call.isCalled(Chroot) || Call.isCalled(Chdir))
+  if (matchesAny(Call, Chroot, Chdir))
     return;
 
   // If jail state is ROOT_CHANGED, generate BugReport.
   void *const* k = C.getState()->FindGDM(ChrootChecker::getTag());
   if (k)
     if (isRootChanged((intptr_t) *k))
       if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
         if (!BT_BreakJail)

clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

 bool InnerPointerChecker::isInvalidatingMemberFunction(
         const CallEvent &Call) const {
   if (const auto *MemOpCall = dyn_cast<CXXMemberOperatorCall>(&Call)) {
     OverloadedOperatorKind Opc = MemOpCall->getOriginExpr()->getOperator();
     if (Opc == OO_Equal || Opc == OO_PlusEqual)
       return true;
     return false;
   }
-  return (isa<CXXDestructorCall>(Call) || Call.isCalled(AppendFn) ||
-          Call.isCalled(AssignFn) || Call.isCalled(ClearFn) ||
-          Call.isCalled(EraseFn) || Call.isCalled(InsertFn) ||
-          Call.isCalled(PopBackFn) || Call.isCalled(PushBackFn) ||
-          Call.isCalled(ReplaceFn) || Call.isCalled(ReserveFn) ||
-          Call.isCalled(ResizeFn) || Call.isCalled(ShrinkToFitFn) ||
-          Call.isCalled(SwapFn));
+  return isa<CXXDestructorCall>(Call) ||
+         matchesAny(Call, AppendFn, AssignFn, ClearFn, EraseFn, InsertFn,
+                    PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,
+                    ShrinkToFitFn, SwapFn);
 }
 
 bool InnerPointerChecker::isInnerPointerAccessFunction(
     const CallEvent &Call) const {
-  return (Call.isCalled(CStrFn) || Call.isCalled(DataFn) ||
-          Call.isCalled(DataMemberFn));
+  return matchesAny(Call, CStrFn, DataFn, DataMemberFn);
 }
 
 void InnerPointerChecker::markPtrSymbolsReleased(const CallEvent &Call,
                                                  ProgramStateRef State,
                                                  const MemRegion *MR,
                                                  CheckerContext &C) const {
   if (const PtrSet *PS = State->get<RawPtrMap>(MR)) {
     const Expr *Origin = Call.getOriginExpr();
for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
       SVal Arg = FC->getArgSVal(ArgI);
       const auto *ArgRegion =
           dyn_cast_or_null<TypedValueRegion>(Arg.getAsRegion());
       if (!ArgRegion)
         continue;
 
       // std::addressof function accepts a non-const reference as an argument,
       // but doesn't modify it.
-      if (Call.isCalled(AddressofFn))
+      if (AddressofFn.matches(Call))
         continue;
 
       markPtrSymbolsReleased(Call, State, ArgRegion, C);
     }
   }
 }
 
 // [string.require]

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

if (const auto *MD = dyn_cast<CXXMethodDecl>(D))
     for (const auto *OMD: MD->overridden_methods())
       if (OMD->hasAttr<MIGServerRoutineAttr>())
         return true;
 
   return false;
 }
 
 void MIGChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const {
-  if (Call.isCalled(OsRefRetain)) {
+  if (OsRefRetain.matches(Call)) {
     // If the code is doing reference counting over the parameter,
     // it opens up an opportunity for safely calling a destructor function.
     // TODO: We should still check for over-releases.
     if (const ParmVarDecl *PVD =
             getOriginParam(Call.getArgSVal(0), C, /*IncludeBaseRegions=*/true)) {
       // We never need to clean up the program state because these are
       // top-level parameters anyway, so they're always live.
       C.addTransition(C.getState()->add<RefCountedParameters>(PVD));
     }
     return;
   }
 
   if (!isInMIGCall(C))
     return;
 
   auto I = llvm::find_if(Deallocators,
                          [&](const std::pair<CallDescription, unsigned> &Item) {
-                           return Call.isCalled(Item.first);
+                           return Item.first.matches(Call);
                          });
   if (I == Deallocators.end())
     return;
 
   ProgramStateRef State = C.getState();
   unsigned ArgIdx = I->second;
   SVal Arg = Call.getArgSVal(ArgIdx);
   const ParmVarDecl *PVD = getOriginParam(Arg, C);

clang/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp

 }
 
 int MmapWriteExecChecker::ProtWrite = 0x02;
 int MmapWriteExecChecker::ProtExec  = 0x04;
 int MmapWriteExecChecker::ProtRead  = 0x01;
 
 void MmapWriteExecChecker::checkPreCall(const CallEvent &Call,
                                          CheckerContext &C) const {
-  if (Call.isCalled(MmapFn) || Call.isCalled(MprotectFn)) {
+  if (matchesAny(Call, MmapFn, MprotectFn)) {
     SVal ProtVal = Call.getArgSVal(2);
     Optional<nonloc::ConcreteInt> ProtLoc = ProtVal.getAs<nonloc::ConcreteInt>();
     int64_t Prot = ProtLoc->getValue().getSExtValue();
     if (ProtExecOv != ProtExec)
       ProtExec = ProtExecOv;
     if (ProtReadOv != ProtRead)
       ProtRead = ProtReadOv;
 

clang/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp

LeakBugType.reset(
                   /*SuppressOnSink=*/true));
 }
 
 void SimpleStreamChecker::checkPostCall(const CallEvent &Call,
                                         CheckerContext &C) const {
   if (!Call.isGlobalCFunction())
     return;
 
-  if (!Call.isCalled(OpenFn))
+  if (!OpenFn.matches(Call))
     return;
 
   // Get the symbolic value corresponding to the file handle.
   SymbolRef FileDesc = Call.getReturnValue().getAsSymbol();
   if (!FileDesc)
     return;
 
   // Generate the next transition (an edge in the exploded graph).
   ProgramStateRef State = C.getState();
   State = State->set<StreamMap>(FileDesc, StreamState::getOpened());
   C.addTransition(State);
 }
 
 void SimpleStreamChecker::checkPreCall(const CallEvent &Call,
                                        CheckerContext &C) const {
   if (!Call.isGlobalCFunction())
     return;
 
-  if (!Call.isCalled(CloseFn))
+  if (!CloseFn.matches(Call))
     return;
 
   // Get the symbolic value corresponding to the file handle.
   SymbolRef FileDesc = Call.getArgSVal(0).getAsSymbol();
   if (!FileDesc)
     return;
 
   // Check if the stream has already been closed.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

bool SmartPtrModeling::evalCall(const CallEvent &Call,
   // we can try this function
   if (ModelSmartPtrDereference && isPotentiallyComparisionOpCall(Call))
     if (handleComparisionOp(Call, C))
       return true;
 
   if (ModelSmartPtrDereference && isStdOstreamOperatorCall(Call))
     return handleOstreamOperator(Call, C);
 
-  if (Call.isCalled(StdSwapCall)) {
+  if (StdSwapCall.matches(Call)) {
     // Check the first arg, if it is of std::unique_ptr type.
     assert(Call.getNumArgs() == 2 && "std::swap should have two arguments");
     const Expr *FirstArg = Call.getArgExpr(0);
     if (!smartptr::isStdSmartPtr(FirstArg->getType()->getAsCXXRecordDecl()))
       return false;
     return handleSwap(State, Call.getArgSVal(0), Call.getArgSVal(1), C);
   }
 
-  if (Call.isCalled(StdMakeUniqueCall) ||
-      Call.isCalled(StdMakeUniqueForOverwriteCall)) {
+  if (matchesAny(Call, StdMakeUniqueCall, StdMakeUniqueForOverwriteCall)) {
     if (!ModelSmartPtrDereference)
       return false;
     
     const Optional<SVal> ThisRegionOpt = Call.getReturnValueUnderConstruction();
     if (!ThisRegionOpt)
       return false;
 
     const auto PtrVal = C.getSValBuilder().getConjuredHeapSymbolVal(

clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp

class StringChecker : public Checker<check::PreCall> {
   bool isCharToStringCtor(const CallEvent &Call, const ASTContext &ACtx) const;
 
 public:
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
 };
 
 bool StringChecker::isCharToStringCtor(const CallEvent &Call,
                                        const ASTContext &ACtx) const {
-  if (!Call.isCalled(TwoParamStdStringCtor))
+  if (!TwoParamStdStringCtor.matches(Call))
     return false;
   const auto *FD = dyn_cast<FunctionDecl>(Call.getDecl());
   assert(FD);
 
   // See if we already cached it.
   if (StringConstCharPtrCtor && StringConstCharPtrCtor == FD)
     return true;
 

clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp

const CallDescription
     ValistChecker::VaCopy("__builtin_va_copy", 2),
     ValistChecker::VaEnd("__builtin_va_end", 1);
 } // end anonymous namespace
 
 void ValistChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
   if (!Call.isGlobalCFunction())
     return;
-  if (Call.isCalled(VaStart))
+  if (VaStart.matches(Call))
     checkVAListStartCall(Call, C, false);
-  else if (Call.isCalled(VaCopy))
+  else if (VaCopy.matches(Call))
     checkVAListStartCall(Call, C, true);
-  else if (Call.isCalled(VaEnd))
+  else if (VaEnd.matches(Call))
     checkVAListEndCall(Call, C);
   else {
     for (auto FuncInfo : VAListAccepters) {
-      if (!Call.isCalled(FuncInfo.Func))
+      if (!FuncInfo.Func.matches(Call))
         continue;
       bool Symbolic;
       const MemRegion *VAList =
           getVAListAsRegion(Call.getArgSVal(FuncInfo.VAListPos),
                             Call.getArgExpr(FuncInfo.VAListPos), Symbolic, C);
       if (!VAList)
         return;
 

clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp

 
 public:
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
 };
 } // namespace
 
 void PutenvWithAutoChecker::checkPostCall(const CallEvent &Call,
                                           CheckerContext &C) const {
-  if (!Call.isCalled(Putenv))
+  if (!Putenv.matches(Call))
     return;
 
   SVal ArgV = Call.getArgSVal(0);
   const Expr *ArgExpr = Call.getArgExpr(0);
   const MemSpaceRegion *MSR = ArgV.getAsRegion()->getMemorySpace();
 
   if (!isa<StackSpaceRegion>(MSR))
     return;

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

ProgramPoint CallEvent::getProgramPoint(bool IsPreVisit,
   assert(D && "Cannot get a program point without a statement or decl");
 
   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 {
-  return CD.matches(*this);
-}
-
 SVal CallEvent::getArgSVal(unsigned Index) const {
   const Expr *ArgE = getArgExpr(Index);
   if (!ArgE)
     return UnknownVal();
   return getSVal(ArgE);
 }
 
 SourceRange CallEvent::getArgSourceRange(unsigned Index) const {

clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp

 using namespace ento;
 
 namespace {
 class EvalCallBase : public Checker<eval::Call> {
   const CallDescription Foo = {"foo", 0};
 
 public:
   bool evalCall(const CallEvent &Call, CheckerContext &C) const {
-    return Call.isCalled(Foo);
+    return Foo.matches(Call);
   }
 };
 
 class EvalCallFoo1 : public EvalCallBase {};
 class EvalCallFoo2 : public EvalCallBase {};
 void addEvalFooCheckers(AnalysisASTConsumer &AnalysisConsumer,
                         AnalyzerOptions &AnOpts) {
   AnOpts.CheckersAndPackages = {{"test.EvalFoo1", true},

clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp

 };
 
 template <class Visitor>
 class StatefulChecker : public Checker<check::PreCall> {
   mutable std::unique_ptr<BugType> BT;
 
 public:
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
-    if (Call.isCalled(CallDescription{"preventError", 0})) {
+    if (CallDescription{"preventError", 0}.matches(Call)) {
       C.addTransition(C.getState()->set<ErrorPrevented>(true));
       return;
     }
 
-    if (Call.isCalled(CallDescription{"allowError", 0})) {
+    if (CallDescription{"allowError", 0}.matches(Call)) {
       C.addTransition(C.getState()->set<ErrorPrevented>(false));
       return;
     }
 
-    if (Call.isCalled(CallDescription{"error", 0})) {
+    if (CallDescription{"error", 0}.matches(Call)) {
       if (C.getState()->get<ErrorPrevented>())
         return;
       const ExplodedNode *N = C.generateErrorNode();
       if (!N)
         return;
       if (!BT)
         BT.reset(new BugType(this->getCheckerName(), "error()",
                              categories::SecurityError));