NoQ accepted this revision.
NoQ added a comment.
Looks great!
================
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 =
----------------
xazax.hun wrote:
> 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?
The analyzer behaves pretty badly with default arguments; we don't really model
them unless they are like plain integer literals. But that's not an issue here
because a default parameter/argument is still both a parameter and an argument
(where the argument expression takes the form of `CXXDefaultArgExpr`).
================
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() ||
----------------
rnkovacs wrote:
> 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?
Mmm mmmmm right, dunno. I thought `operator>>` would be our main example, but
it's apparently non-member for a string, even though it is defined as a member
for primitive types.
I guess let's skip the test until we find an example. We did our best to work
around potential future problems, that's good.
================
Comment at: test/Analysis/inner-pointer.cpp:41-42
+template< class T >
+void func_ref(T& a);
+
----------------
rnkovacs wrote:
> 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.
Mm, aha, ok, i misunderstood this test. I thought you're trying to define
something like `std::function` and try to see if calling a function pointer
wrapped into that thing works. No idea why i was thinking that, sry.
https://reviews.llvm.org/D49656
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
