[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Small comments inline.



================
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:181
 
-  auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
-  if (TypeDecl->getName() != "basic_string")
-    return;
+    for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+      QualType ParamTy = FD->getParamDecl(I)->getType();
----------------
Nit: maybe using `FunctionDecl::parameters` and range based for loop is sligtly cleaner.


================
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:189
+      // argument but not as a parameter.
+      const auto *MemberOpCall = dyn_cast<CXXMemberOperatorCall>(FC);
+      unsigned ArgI = MemberOpCall ? I+1 : I;
----------------
Nit: maybe using isa + bool is cleaner here?


================
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:192
 
-  if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
-    SVal RawPtr = Call.getReturnValue();
-    if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
-      // Start tracking this raw pointer by adding it to the set of symbols
-      // associated with this container object in the program state map.
-      PtrSet::Factory &F = State->getStateManager().get_context<PtrSet>();
-      const PtrSet *SetPtr = State->get<RawPtrMap>(ObjRegion);
-      PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
-      assert(C.wasInlined || !Set.contains(Sym));
-      Set = F.add(Set, Sym);
-      State = State->set<RawPtrMap>(ObjRegion, Set);
-      C.addTransition(State);
+      SVal Arg = FC->getArgSVal(ArgI);
+      const auto *ArgRegion =
----------------
While I cannot recall examples in the STL the number of arguments and parameters might differ. We might have ellipsis in the parameters and this way we may pass more arguments. Since we do not have the parameter type for this case, I think it is ok to not handle it. But it might be worth to have a comment. The other case, when we have default arguments. I do not really know how the analyzer behaves with default arguments but maybe it would be worth to add a test case to ensure we do not crash on that?


https://reviews.llvm.org/D49656