New check added to the checker which checks whether iterator parameters of template functions typed by the same template parameter refer to the same container.
NoQ dcoughlin george.karpenkov
- rG21583b733af1: [Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function…
rL341790: [Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function…
rC341790: [Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function…
This looks like a nice improvement. I have some remarks inline.
The function becomes > 100 lines long. Should we refactor this check into a separate function to improve readability?
While this assumption is sane and is true for <algorithm> functions, user code can have other design solutions. There is nothing that prevents users from writing a function looking like:
template <typename IterTy> void f(IterTy FromBegin, IterTy FromEnd, IterTy ToBegin, IterTy ToEnd);
and there is nothing wrong with it.
size_t I = 0?
We always report about first iterator, but the mismatched one can be second. I think this deserves a FIXME, at least.
We usually pass StringRefs and SVals by value because they're very cheap for copying. However, the surrounding code follows the same conventions so it's not strongly required to change.
Yes, I think so this would be a good idea. Should I do it now?
We can restrict, of course, but first we should measure how it performs on real code. With the restriction, we can get rid of some false positives but we may also loose some true positives.
Is there any particular order entries of this file should be in? Seems to be broken now, but I guess when this patch comes up to the top of the stack it shall be fixed at a rebase.
functions' parameters' ?
If this string is the message that gets printed to the user, I think it must be rephrased a bit. If this message came across me, I'd be puzzled at first to understand what it is trying to mean.
One more thing:
The main purpose of iterators is to make algorithms independent of the data representation. So you can decide whether your algorithm works on a specific representation and create non-template function that takes reference for the specific container itself or you make it generic so you use template function which takes iterators. If you chose this latter one for an algorithm that works on two different container then there is no point to restrict the function to only work on two containers with exactly the same representation. Either specific or generic, but there is no point for something in between.
Since rL338263 fixed a bug in the cleanup phase the tests for mismatched iterator checker did not pass. The reason for this is that the region of some LazyCompoundVals are cleaned up while there are still iterator positions connected to the LazyCompoundVal itself. This happens typically for arguments which are constructed in-place (e.g. begin() or end() of a container is invoked in the argument itself).
We applied a fix here that defers cleanup of such iterator positions. No other solution comes to my mind at the moment.
I wanted to upload this fix in a separate patch but I could not create tests for it.
@NoQ please review this fix before I commit the patch.