Re: [PATCH] disas/nanomips: Convert nanoMIPS disassembler to C

[Peter Maydell]

On Mon, 1 Aug 2022 at 04:18, Vince Del Vecchio
<Vince.DelVecchio@mediatek.com> wrote:
>
> On Fri, 29 Jul 2022 at 10:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> > ...
> > Is it possible to break this down into smaller pieces so it isn't one 
> > single enormous 5000 line patch ?
> >
> > I guess partial conversion is likely to run into compilation difficulties 
> > mid-series; if so we could do "disable the disassembler; convert it; 
> > reenable it".
> >
> > The rationale here is code review -- reviewing a single huge patch is 
> > essentially impossible, so it needs to be broken down into coherent smaller 
> > pieces to be reviewable.

> Something like 90% of the patch is purely mechanical transformations of which 
> I've excerpted two examples below.  (There are 3-4 chunks of mechanical 
> transformations, of which I've shown the most complicated type that accounts 
> for ~60% of the changed lines.)  Did you intend to review this by having a 
> human inspect all of these?

If they're mechanical transformations then:
 * the commit message should say what the mechanical transformation is
   (ie quote the sed script or other mechanism used to create the commit)
 * where sensible, different transformations should be in different
   commits
 * the hand-implemented parts should be separate

And yes, review means human inspection. You need to make that
human inspection easy by separating out the stuff that is
"just change std::string to const char *" so that the human can
do a 30-second skim over that patch, and spend the time on
the patch containing the stuff that's more complicated.

-- PMM