> On 2011-05-28 22:04:00, Steve Reinhardt wrote: > > This looks fine to me, but I'm confused... don't you delete this code > > completely two patches from now? Why bother changing it if you're going to > > get rid of it? > > Gabe Black wrote: > Because this way both are available, and the ISAs can be converted one at > a time and everything will work between every patch. Otherwise I'd have to > break everything that wasn't (or was) updated, or do everything in one big > change that does too much at once. Once all the ISAs are consistently on the > new system, the old system isn't needed anymore and is deleted in that later > patch. > > Steve Reinhardt wrote: > OK, I see. Overall this seems like overkill; I can see how this code was > useful while you were developing, but for committing, one complete patch that > gets rid of the old way of doing things and fixes all the ISAs to use the new > way would be preferable to me. I think it would be less confusing because > all the related changes would be right there in one place. I don't have a > problem with large changesets (in terms of lines of code or files touched), > just ones that are not conceptually unified, and spreading this change across > multiple csets goes against "conceptually unified" in the opposite direction, > IMO.
That makes sense, but then I think when changes get to be too big, they get to be too hard to understand all at once. By keeping each part relatively small it makes things easier to digest later. My enormous PC state change is an example of the opposite extreme, and it was probably a lot of work to get through and review and overwhelming to look at later in the history. I'd like to avoid that if possible. - Gabe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/656/#review1277 ----------------------------------------------------------- On 2011-04-25 03:04:12, Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/656/ > ----------------------------------------------------------- > > (Updated 2011-04-25 03:04:12) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > ISA parser: Allow defining operand types with a ctype directly. > > > Diffs > ----- > > src/arch/isa_parser.py de679a068dd8 > > Diff: http://reviews.m5sim.org/r/656/diff > > > Testing > ------- > > > Thanks, > > Gabe > > _______________________________________________ gem5-dev mailing list gem5-dev@m5sim.org http://m5sim.org/mailman/listinfo/gem5-dev
