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

[Fergus Henderson]

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".

OK, those four should go together.

> 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".

Well, you have to weigh up the cost of the disruption against the increased
difficulty of reviewing.

There are other ways of minimizing the disruption, e.g. by doing the
first change, getting it reviewed, then doing the second change and
getting that reviewed, and only then committing both changes
(as separate commits but at the same time).  Or you could commit
onto a branch.

> 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".

Merging changes simply because they happen to affect the same line
of code is not a great idea, IMHO.

> 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.

No, you've misunderstood.  I always *point out* any problems that I happen
to notice in the original code while reviewing a change.  That is a quite
different thing to objecting if someone doesn't fix those problems
while making some unrelated change.

> So you have to make up your minds. Do you want misc cleanups to be included,
> or not?

I would prefer it if misc cleanups are submitted as separate diffs.

> Do you want small, easy-to-review changes, or do you want complete
> changes?

I want complete changes that are as small and easy-to-review as possible.
These objectives are not mutually incompatible.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.