[m-dev.] for review: the new debugger command set (part 1 of 5)

[Tyson Dowd]

On 21-Sep-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> > My biggest criticism is that this is an enormous change, and would
> > be much better reviewed as a few different changes.
> > 
> > The problem is you are not going to get very detailed comments on a
> > particular change, because there are 17 separate changes whose diffs are
> > intertwined, so it is difficult to remember what a particular change was
> > even intended to achieve, let alone whether it is correct.
> > 
> > Here is a list of what I consider to be the separate changes in
> > this diff.
> > 	- Change command set.
> > 	- Add redo events
> > 	- Reduce arguments to MR_trace
> > 	- Renumber framevars
> > 	- Generate online documentation.
> > 	- Add user guide docs for debugging
> > 	- Clean up some documentation
> > 	- Reorganize handling of tracing levels
> > 	- Identify variables separately
> > 	- Fix a bug with return live vals not being computed right.
> > 	- Allow debugger to output to streams other than stdout.
> > 	- Add help.m to the library.
> > 	- Add stack layout info for builtin_aggregate
> > 	- Fix problem with higher order stack layouts.
> > 	- Miscellaneous fixes and additions to the runtime
> > 	- Add trace histogram feature.
> > 	- A few improvements to bootstrap.
> 
> I tried committing some of these as a separate change. It was not approved
> because it was not complete. In particular, Fergus rejected my "Change command
> set" change because it did not have "Add user guide docs for debugging",
> Generate online documentation", and "Add help.m to the library".

I am happy with some linked changes.  There do not need to be 17
different commits, just a few would be fine.

I would even tolerate this all being committed together.
But I will not let it pass without comment, because it is the
wrong thing to do.

> 
> Other changes are also linked. "Add redo events" requires changing the
> signature of MR_trace, so it reduces disruption if it is done at the same time
> as "Reduce arguments to MR_trace". It also requires changes the way framevars
> are allocated, so not doing "Renumber framevars" at the same time would require
> the same work being done twice. Ditto for "Reorganize handling of tracing
> levels".

And this is entirely the sort of information that is lost when you
commit a bunch of changes like this together.  Anyone who want  to
see how a change works or doesn't work, or how to merge it successfully,
or how to back it out will need to reconstruct the dependencies like
this.  

I have no problem with dependent or releated changes being checked in
together.

Documentation, redo events, variable identification, and bug fixes


> 
> As for the misc cleanups, Fergus always objects if a diff submitted by anyone
> shows that the *original* code had some problem that the diff did not fix.

I have no problem with misc cleanups being commited with the code they
are near, although obviously it would be nice if they could be
separated.  (Merging in formatting changes alongside functional changes
is always very streesful and error-prone because it is difficult to
resolve the conflicts).

> 
> So you have to make up your minds. Do you want misc cleanups to be included,
> or not? Do you want small, easy-to-review changes, or do you want complete
> changes?

A compromise will be fine.  Clearly we have both indicated that this is
too much at once -- you could probably have halved it into
"documentation" and "everything else" and got it past without comment.


> 
> If this sounds testy, it's because I am.

That's understandable, I didn't think you would actually *like* to
hear this criticism.
But this change is still to big and too eclectic,
and I don't want to encourage a culture of working for 3 months in
a single workspace and then committing it all as one big change.
Even if it was unavoidable, or justifiable, I'd just rather it didn't
happen.

It feels like the government system of tacking unrelated amendments onto
important bills in the hope that it will all get past at once.  Now
we have to review this change as if it is one entity, and it becomes
tempting to let big problems like the online help runtime dependency on the
library past so we can get all the other changes through the system.

As it is, I could say "this is fine, commit" to about half the changes,
but the other half need some discussion, and some might need another
review.  I don't want to have to review the whole thing again, but I
will have to because it's almost inevitable that you will need to
change more of the half that is fine to fix the half that is not fine,
so the diff will just grow.  Eventually we will have to just say "oh,
commit it anyway, and we will fix the problems later".

So perhaps we can split off the "redo event" change from the online
documentation change at least, and then work on getting them commited
separately.

-- 
Those who would give up essential liberty to purchase a little temporary
safety deserve neither liberty nor safety.     - Benjamin Franklin

Tyson Dowd   <tyson at tyse.net>   http://tyse.net