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

[Reka Kovacs via Phabricator via cfe-commits]

rnkovacs added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:207-208
+
+    for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+      QualType ParamTy = FD->getParamDecl(I)->getType();
+      if (!ParamTy->isReferenceType() ||
----------------
NoQ wrote:
> NoQ wrote:
> > We need tests for operators here, due to the problem that i recently encountered in D49627: there's different numbering for arguments and parameters within in-class operators. Namely, this-argument is counted as an argument (shifting other argument indices by 1) but not as a parameter.
> (i.e., tests and most likely some actual code branch)
I did the coding part, but for the tests, I'm struggling to find an example of a member operator in the STL that takes a non-const string reference as an argument. Could you perhaps help me out here?


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2934
+            } else if (const auto *CallE = dyn_cast<CallExpr>(S)) {
+              OS << CallE->getDirectCallee()->getNameAsString();
             }
----------------
NoQ wrote:
> rnkovacs wrote:
> > xazax.hun wrote:
> > > I think `getDirectCallee` might fail and return `nullptr`. One more reason to test function pointers :)
> > You're right. Also, it needed a bit more effort to dig up the function pointer's name. Or should I go further and somehow find out the name of the function it points to?
> > Also, it needed a bit more effort to dig up the function pointer's name.
> 
> I think it should work out of the box; `Call.getDecl()` is smart enough to show the right decl when a pointer to a concrete function is being called on the current path.
That worked, thanks!


================
Comment at: test/Analysis/inner-pointer.cpp:41-42
 
+template< class T >
+void func_ref(T& a);
+
----------------
NoQ wrote:
> Without a definition for this thing, we won't be able to test much. I suggest you to take a pointer to a function directly, i.e. define a `std::swap<T>(x, y)` somewhere and take a `&std::swap<std::string>` directly and call it (hope it actually works this way).
I think I am missing something. This is meant to test that the checker recognizes standard library function calls taking a non-const reference to a string as an argument. At L314, `func_ref` is called with a string argument, and the checker seems to do all the things it should after the current patch, emitting the new kind of note and such.

I can surely add the `std::swap<T>(x, y)` definition too, but then the analyzer will see that the pointer was reallocated at the assignment inside the function, and place the note there. I think that would test the previous string-member-function patch more than this one.


https://reviews.llvm.org/D49656