On Sun, Feb 3, 2013 at 12:02 PM, Jed Brown <jedbrown at mcs.anl.gov> wrote:
> > On Sun, Feb 3, 2013 at 9:35 AM, Matthew Knepley <knepley at gmail.com> wrote: > >> Again, you misuse words when it is convenient for you. This is dishonest >> argument. >> >> "Stable" for applications means that the behavior of the application will >> not change. This is >> definitely true for your example, so characterization as "unstable" is >> plainly wrong. >> > > If a build fails or a memory leak appears, it definitely impacts the > application. In the particular example from yesterday, the build did not > fail, they were "just warnings". But the warnings were from obviously > broken code and cause the application developer to question whether that > code might affect them. This sort of thing erodes trust and willingness to > follow petsc-dev. > If the checkin you originally complained about the build did not fail and a memory leak did not appear. You still cannot explain what was wrong there, so you proceed to a different problem. > Since we make API changes in petsc-dev, people may need to update their > code. For example, I updated parmod last night and had to update a few > things for it to build, but then -malloc_dump told me about a memory leak. > If I was not a PETSc developer, should I have expected it to be due to a > bug in my code or a new bug in PETSc? As it turns out, you introduced the > memory leak as part of a bunch of other stuff > Yes, a memory leak did appear here. However, it appeared in a complete checkin that had a test to go with it, exactly as you ask. We see that even that is insufficient sometimes to discover a minor bug like this. So your second example is really about the shortcomings of the strategy you propose. Matt > > https://bitbucket.org/petsc/petsc-dev/commits/16ecfb2c3de392ee3e460feb23b4342218c52c8a > > Even knowing that component of PETSc well, the one-line commit message > "DMPlex: Serial test for two triangles working for cohesive cell > introduction" sure doesn't suggest that something called by > DMPlexConstructGhostCells was refactored. I fixed the bug quickly > > > https://bitbucket.org/petsc/petsc-dev/commits/2f3734bedc2dade526c09f6b0348515e6a420442 > > but these things can waste a lot more users' time and turn them off of > following petsc-dev. > > > When you push incomplete code as a checkpoint, it discourages the rest of > us from looking at it. Pushing it in that form offers no value at all, and > breaks the coherent thread of a feature. It's using a DVCS with an > almost-CVS workflow. When writing code, we often put more work into it > up-front to make it easier to maintain later. We're writing it not just to > execute, but also to be readable. Coherent history helps us all understand > what is going into PETSc. It helps us review and spot bugs before users > find them. More importantly, the review process improves > maintainability/extensibility [1]. When code is checkpointed haphazardly, > the planned design is often not yet coherent enough to even comment on > whether the design is good. > > The only professional thing to do is to write code not just with the > intent of being readable once "complete", but specifically to be > understandable _incrementally_. This means breaking patches into pieces > that have some meaning on their own and only merging at semantically > meaningful places. > > [1] http://lib.tkk.fi/Diss/2009/isbn9789512298570/article5.pdf (and > others) > > -- What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead. -- Norbert Wiener -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20130203/cda3e01c/attachment.html>