[PATCH] D31975: [Analyzer] Iterator Checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I've seen the new reviews. A lot of thanks for working on the patch split. I believe it'd eventually benefit everyone, similarly to how everybody benefits from code quality or something like that. I'd try to keep up with the "to split or not to split" discussion a bit below, not sure if it's of much interest since we're already trying this out practically and we'll see how much benefit it brings at the end :) And in any case, it's quite hard to see at first that the "incremental" mindset actually works in practice, as i do know that initially it feels strange at times.

In https://reviews.llvm.org/D31975#737751, @baloghadamsoftware wrote:

> Actually, I did neither merge the patches nor the checkers. Based on the experiences in the prototype I created a totally new checker. Incremental development sounds good, but I did a lot of experimenting in this particular checker, which means I added things then removed them, I changed things then changed them back etc.


Yep. In this case, a high-level description of why a rewrite is necessary and what was wrong with the old approach would be really helpful, both after and //before// doing actual work. I'm really interested in what downsides you found in the old checker that needed a complete rewrite to mitigate. Because normally there are very few right ways to write the checker. I've a feeling this experience would be very useful to the community.

While it's definitely not worth it to publish a review every time you're conducting an experiment, it's a good idea to maintain the local history clean and incremental (probably utilizing rebase regularly, eg. if you patch up an old feature, move the patch back to that feature and squash it up after committing), so that splitting it up later was trivial.

In https://reviews.llvm.org/D31975#737751, @baloghadamsoftware wrote:

> I can try to separate that small part (simple_good and simple_bad tests pass), but cutting the patch in 12+ parts is overkill. For example, separation of insert and emplace is pointless because they affect iterators exactly the same way. But I woul also merge them with erase, the patches remain small enough. I think 5 parts are more than enough


Yep, that's right, i definitely don't insist on re-using my examples; they were done in a rush and blindly and often don't look harmonously.

In https://reviews.llvm.org/D31975#737751, @baloghadamsoftware wrote:

> because review will take weeks, so uploading them incrementally one-by-one will take at least half a year.


I believe it doesn't work that way :)

- Smaller changes are non-linearly easier to review; large changes require keeping all the stuff in your head, and you cannot move forward with the review until you understand all of it. Ultimately, reducing per-line-of-code workload on us would allow us to do more reviews and keep contributors happy.
  - In my experience, it especially applies to estimating test coverage - it would be hard to notice what features are not covered by tests because you need to keep all the features in your head simultaneously. I think i've noticed some strange things in the coverage when i was splitting up the patches, but i didn't record these anywhere.
- Small reviews can be completed in a few hours, maybe even in minutes, of continuous work, but large reviews require large continuous time slots; for patches with thousands of lines of code changed, we may never be able to find such time slots.
- Small reviews increase the chances that at least a part of the contributor's work lands. If some patches are delayed because an alternative approach is proposed, probably involving changes that you're not taking up, we can at least have the rest of the changes in.
- Small reviews would attract more reviewers, who may not be ready to review large changes, or are only sure about their skill in some technologies used but not in other.
- Because our contributors are contributing large changes, we have to apply review pressure on them to make sure these changes are complete and self-contained, because we cannot be sure they will be maintained by the author later. We'd much more eagerly accept a smaller change when the author says he'd fix things up in follow-up commits, even if these changes are obviously incomplete and lacking features. So the review for smaller changes is ultimately less strict.

So, ideally, i believe that reviews aren't normally slow; the impression that reviews are slow only appears when many authors try to contribute large patches.

Sorry for the ranting, and thanks again for investing more and more time into the Analyzer><


https://reviews.llvm.org/D31975