> 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. > > Gabe Black wrote: > 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 Black wrote: > Then again, the changes after this one don't add much complexity to this > one since they're pretty simple. Merging them is reasonable, and I'll do that > once we're ready to put these in.
Yea, that's what I was thinking... while patches can get pretty big, if you just combine the ISA-specific operand_type fixes (like http://reviews.m5sim.org/r/657, but basically doing the same thing to all the ISAs) with the one that permanently changes how the operand_type block is processed (http://reviews.m5sim.org/r/658, which basically makes this one irrelevant), then it's still not that big. I'm definitely not suggesting that you fold in http://reviews.m5sim.org/r/655, just that you combine this one (656) with 657 (& siblings) and 658. - Steve ----------------------------------------------------------- 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
