[llvm] r213474 - [C++11] Add predecessors(BasicBlock *) / successors(BasicBlock *) iterator ranges.

[Manuel Jacob]

On 2014-07-21 22:51, Chandler Carruth wrote:
> On Mon, Jul 21, 2014 at 10:31 AM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
> 
>> BTW, I don't see the value in these mass refactorings to use C++11
>> range-based for loops.  While adding the API is obviously good,
>> changing
>> everything to use it means that these mistakes are going to creep
>> in,
>> and it's hard enough to review these that they won't get many eyes.
>> 
>> I think it would be better if the next person to touch the code
>> cleaned
>> it up (where appropriate).  If they're touching the code anyway,
>> they'll
>> have the time to understand the logic and be careful about it.
> 
> FWIW, I *strongly* agree.
> 
> There was one place where I updated the users of the API and that was
> because *all of them had to change anyways*. And it was miserable,
> horrid, and required a ton of testing and review to shake out the
> bugs.
> 
> Please, don't "fix" code in ways that can introduce subtle bugs unless
> you're working on that code anyways, and do so in small chunks that
> are easier to test, revert, and isolate when bugs sneak in.

Thank you both for pointing that out.  I realize that these 
patches/commits are hard to review and test.  Usually I contribute to a 
project with very good test coverage and which is also written in a 
language which fails more loudly in cases like this.  Sorry for causing 
trouble and wasting your's and Takumi's time to find the bug.  I'll be 
more careful and don't solely rely on running the test suite in the 
future.

Should I recommit the API addition?

-Manuel