> On 2010-08-23 06:59:18, Nathan Binkert wrote: > > Overall, I have to say that this is a much better way to do things. Did > > you consider using the Flags class in src/base/flags.hh? There are some > > examples of how it's used in sim/eventq.hh and mem/packet.hh. I'm not sure > > if it's overkill or not. I'm also not sure if adding any checking to the > > setting/getting of flags is worth it, but I thought I'd point it out. > > Also, setFlags as a parameter name seems strange. Why not just call it > > "flags"?
I thought about using the Flags class, but it seemed like overkill like you said. I also considered adding some sort of check that would prevent bad flags, contradictory flags, etc., but that seemed like it'd be hard to maintain. I chose setFlags because it's really just the flags you want to ensure are set. If there are other flags already set automatically for some reason then you wouldn't be able to clear them with that parameter. - Gabe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/205/#review286 ----------------------------------------------------------- On 2010-08-22 18:53:02, Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/205/ > ----------------------------------------------------------- > > (Updated 2010-08-22 18:53:02) > > > Review request for Default. > > > Summary > ------- > > X86: Consolidate extra microop flags into one parameter. > > This single parameter replaces the collection of bools that set up various > flavors of microops. A flag parameter also allows other flags to be set like > the serialize before/after flags, etc., without having to change the > constructor. > > > Diffs > ----- > > src/arch/x86/insts/microfpop.hh 405f840c4ae1 > src/arch/x86/insts/microldstop.hh 405f840c4ae1 > src/arch/x86/insts/micromediaop.hh 405f840c4ae1 > src/arch/x86/insts/microop.hh 405f840c4ae1 > src/arch/x86/insts/microregop.hh 405f840c4ae1 > src/arch/x86/isa/macroop.isa 405f840c4ae1 > src/arch/x86/isa/microops/base.isa 405f840c4ae1 > src/arch/x86/isa/microops/debug.isa 405f840c4ae1 > src/arch/x86/isa/microops/fpop.isa 405f840c4ae1 > src/arch/x86/isa/microops/ldstop.isa 405f840c4ae1 > src/arch/x86/isa/microops/limmop.isa 405f840c4ae1 > src/arch/x86/isa/microops/mediaop.isa 405f840c4ae1 > src/arch/x86/isa/microops/regop.isa 405f840c4ae1 > src/arch/x86/isa/microops/seqop.isa 405f840c4ae1 > src/arch/x86/isa/microops/specop.isa 405f840c4ae1 > > Diff: http://reviews.m5sim.org/r/205/diff > > > Testing > ------- > > > Thanks, > > Gabe > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
