[PATCH] D31975: [Analyzer] Iterator Checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I had a look at how hard it is to split the patch.

In ~3 hours i reached this far:

F3239478: 0001-Delete-the-old-checker.patch <https://reviews.llvm.org/F3239478>
F3239481: 0002-Reduced-patch-with-the-following-changes.patch <https://reviews.llvm.org/F3239481>
F3239475: 0003-Add-support-for-iterator-invalidation-on-assignments.patch <https://reviews.llvm.org/F3239475>
F3239476: 0004-Add-support-for-push_back.patch <https://reviews.llvm.org/F3239476>
F3239474: 0005-Add-MismatchedIteratorChecker.patch <https://reviews.llvm.org/F3239474>
F3239473: 0006-Add-support-for-mismatched-iterators-in-constructors.patch <https://reviews.llvm.org/F3239473>
F3239477: 0007-Add-support-for-mismatched-iterators-in-comparisons.patch <https://reviews.llvm.org/F3239477>
F3239479: 0008-Add-support-for-insert.patch <https://reviews.llvm.org/F3239479>
F3239480: 0009-Add-support-for-emplace.patch <https://reviews.llvm.org/F3239480>
F3239482: 0010-Add-support-for-erase.patch <https://reviews.llvm.org/F3239482>
F3239484: 0011-Add-evalCall.patch <https://reviews.llvm.org/F3239484>
F3239483: 0012-Add-checkBeginFunction.patch <https://reviews.llvm.org/F3239483>

That job is around 40% done; in patch 0002, the checker has around 1.5k lines of code. Would you be willing to continue this? It should be possible to go on splitting up features until there's just one checker under, say, maybe ~500 lines of code (blind guess), with the only positive being `simple_bad_end()` in `iterator-range.cpp`.

My workflow was like this:

1. Put the original patch into a git branch.
2. Choose a feature that seems relatively independent. Good canindates for such features are:
  - Support for particular API.
  - Any particular hack.
  - Any particular rule to check in the checker.
  - The checker itself when it's already small enough.
3. Remove the feature and all tests that start failing.
  - Watch out for "unused function" compiler warnings.
  - Watch out for related tests, eg. remove "good" when you remove "bad".
  - Watch out for the system header simulator:
    - You'd need to update `diagnostics/explicit-suppression.cpp` test every time.
    - See if what you're removing was actually added by you, and no unrelated analyzer tests fail when you remove it.
  - This step is quite blind. If something is too interdependent, pick another feature.
4. Commit the removal to your branch.
5. Go to 2 unless satisfied with the work you've done.
6. Mass revert all removal commits in the opposite order. Revert of removal is addition, hence the revert-commits can go on review.
7. Mass squash all removal commits into the original commit. That'd be the first commit to go on review.

If you continue this, you'd take my patches 0001 and 0002 instead of your original patch, follow the same method, and then once you're done, add my other patches.

I believe the next good steps would be to remove the invalidated iterator checker (together with support for the `clear()` API) and then probably try to split out the support for iterator comparisons or support for increments/decrements, then maybe separate access checks from dereference checks.

Once this is done, it'd be great to have a separate review for each patch; it'd put a bit of stress on you to do the rebases once you address comments on the older patches, but that'd make everybody who wants to review the patch, either here on phabricator, or after the commit, a lot happier. I understand that it's not entirely a pleasant thing to do; i also understand that i might have caused confusion by asking you to merge checkers, while i didn't really mean to ask you to merge the //patches// as well; but it seems that it is universally accepted around there that changes should be small, so i guess it'd worth it.

Note that //the point of incremental development is//, of course, //**not** splitting up huge patches before submitting to the review//; the point is to actually start developing the second small patch only after the first small patch is published. Like, it's perfectly fine to submit an unfinished checker that does 10% of what you want and finds just one tiny bug and has completely obvious false positives due to lack of various modeling, as long as you say "i'd add these features in a follow-up patch". But now that we'd have to incrementally develop retroactively, it is still considered to be better than having huge commits.


https://reviews.llvm.org/D31975