----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/8/#review11 -----------------------------------------------------------
How often do you use Clock without ClockTicker? It seems like ClockTicker doesn't really have state, in a sense, it's just a cache for not having to recalculate tick/cycle values near curTick (or if it's not exactly that, does it make sense to make it that?) Also, I'm not sure it's worth it, but did you consider having a base Clock class that doesn't have a phase? How often is the phase non-zero anyway? src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment52> *All* of the classes need comments... you've already got decent descriptions in the patch description, just add them as comments in the code too. src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment43> Please document these two fields. src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment48> Please document this method src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment49> Isn't this equivalent to the next three statements: // Find the previous unshifted edge Tick edge = unshifted - unshifted % period; // Advance to next edge and shift edge += period + phase; It's more intuitive to me, anyway... src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment44> Please add in 'assert(edge > when)' (or >=) to double-check your math and document the exit condition. Better yet: assert(edge > when && edge <= when + period) src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment47> Shouldn't this comment apply to the whole method, not just this line of code? src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment51> Do we care about dealing with the corner case when (when < phase)? Rounding for negative integer division isn't standardized in C. src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment45> Please document these fields. src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment46> Please document this method. src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment50> This comment doesn't make sense to me in this context. src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment55> What if tick > curTick already? src/base/clock.hh <http://reviews.m5sim.org/r/8/#comment54> Are there other special cases to check here before we do all the math? What about where curTick is between the old tick and new tick values? src/sim/periodic_event.hh <http://reviews.m5sim.org/r/8/#comment53> This flag name isn't quite right... sounds like a pointer var not a flag. How about DeleteTicker? - Steve On 2010-04-18 22:45:31, Nathan Binkert wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/8/ > ----------------------------------------------------------- > > (Updated 2010-04-18 22:45:31) > > > Review request for Default. > > > Summary > ------- > > eventq: add classes for Clock, ClockTicker, and PeriodicEvent > - Clock class is simply a phase and a period and can calculate the > next edge based on these variables > - ClockTicker class keeps track of the current clock cycle and is > intended to be used to schedule events > - PeriodicEvent class uses a ClockTicker to call an event on every > clock edge with simple functions to enable and disable the ticker > > > Diffs > ----- > > src/base/clock.hh PRE-CREATION > src/sim/periodic_event.hh PRE-CREATION > > Diff: http://reviews.m5sim.org/r/8/diff > > > Testing > ------- > > > Thanks, > > Nathan > > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
