[Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.

[Jim Ingham via lldb-commits]

Another principle that seems useful here is not to make this sort of logic change unless this is code you are currently working on.  When you have the actual purpose of the code in your head, it is fairly easy to make this sort of change correctly.  You know what the code is supposed to be doing, and so it's easy to make sure the new form preserves that purpose.  And if you are working on it you either know that it is tested or plan to add tests to it, which makes success even more likely.  But when you are just skimming and happen to see something that you think isn't in the right form, you don't have adequate context, and you are much more likely to make a mistake.  

Jim




> On Feb 28, 2017, at 3:22 PM, Jim Ingham via lldb-commits <lldb-commits at lists.llvm.org> wrote:
> 
> 
>> On Feb 28, 2017, at 3:14 PM, Zachary Turner <zturner at google.com> wrote:
>> 
>> On Tue, Feb 28, 2017 at 3:07 PM Jason Molenda <jmolenda at apple.com> wrote:
>> At it's core, lldb is a real world tool that thousands of people depend on; breaking it or introducing bugs for little gain beyond aesthetics is a very poor tradeoff.  
>> 
>> Just for the record, I disagree with this assertion that there is little gain beyond aesthetics (as does I think almost everyone else in the LLVM/LLDB community).
> 
> 
> No doubt, early returns makes it easier to reason about complicated code.  But you added an early return to a function that had maybe 10 lines of code in it and was trivial to read either way.  There was pretty much zero chance somebody working on the code before this change would introduce a bug that they wouldn't because of the clarity provided by the early return.  But in doing so you DID add a bug.  In this case it seems clear that for the sake of very little more than aesthetics, you introduced a bug.  That seems to me a very poor tradeoff.
> 
> BTW, somebody at Apple tried to get the llvm version of gcov working on the lldb testsuite to see what kind of coverage we actually had.  It didn't work right off the bat for reasons that weren't clear, and whoever did the initial effort lost the window of time they had to work on this.  But that would be a useful exercise; then you could know whether the code you were touching was already well tested.  Then we could gate any of these sorts of formal manipulations on there being adequate test coverage of the affected area in advance of that work.
> 
> 
> Jim
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits