> On 2010-06-06 12:34:28, Nilay Vaish wrote: > > The changes seem > > > > I have two observations > > > > 1. In several places we are using dynamic_cast. The patch under > > consideration moves some of these to safe_cast and retains dynamic_cast at > > other places. Should we not use safe_cast in all places? This would avoid > > the checks that dynamic_cast carries out when not running the debug version. > > > > 2. In the file src/mem/ruby/system/DMASequencer.cc, we use > > msg->getType/LineAddress/PhysicalAddress() = ; > > Should we not call the set() function instead? Both we will have the same > > effect but set() is more readable. > >
The first should be read as - "The changes seem fine to me." - Nilay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/21/#review35 ----------------------------------------------------------- On 2010-06-02 15:56:26, Nathan Binkert wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/21/ > ----------------------------------------------------------- > > (Updated 2010-06-02 15:56:26) > > > Review request for Ruby Reviewers. > > > Summary > ------- > > ruby: get rid of RefCnt and Allocator stuff use base/refcnt.hh > > This was somewhat tricky because the RefCnt API was somewhat odd. The > biggest confusion was that the the RefCnt object's constructor that > took a TYPE& cloned the object. I created an explicit virtual clone() > function for things that took advantage of this version of the > constructor. I was conservative and used clone() when I was in doubt > of whether or not it was necessary. I still think that there are > probably too many instances of clone(), but hopefully not too many. > > I converted several instances of const MsgPtr & to a simple MsgPtr. > If the function wants to avoid the overhead of creating another > reference, then it should just use a regular pointer instead of a ref > counting ptr. > > There were a couple of instances where refcounted objects were created > on the stack. This seems pretty dangerous since if you ever > accidentally make a reference to that object with a ref counting > pointer, bad things are bound to happen. > > > Diffs > ----- > > src/mem/gems_common/Allocator.hh 9ea24d102d66 > src/mem/gems_common/RefCnt.hh 9ea24d102d66 > src/mem/gems_common/RefCnt_tester.cc 9ea24d102d66 > src/mem/gems_common/RefCountable.hh 9ea24d102d66 > src/mem/gems_common/SConscript 9ea24d102d66 > src/mem/ruby/buffers/MessageBuffer.hh 9ea24d102d66 > src/mem/ruby/buffers/MessageBuffer.cc 9ea24d102d66 > src/mem/ruby/common/Debug.hh 9ea24d102d66 > src/mem/ruby/common/Debug.cc 9ea24d102d66 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc > 9ea24d102d66 > src/mem/ruby/network/garnet/fixed-pipeline/RoutingUnit_d.cc 9ea24d102d66 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.cc > 9ea24d102d66 > src/mem/ruby/network/garnet/flexible-pipeline/Router.cc 9ea24d102d66 > src/mem/ruby/network/simple/PerfectSwitch.cc 9ea24d102d66 > src/mem/ruby/network/simple/Throttle.cc 9ea24d102d66 > src/mem/ruby/slicc_interface/Message.hh 9ea24d102d66 > src/mem/ruby/slicc_interface/NetworkMessage.hh 9ea24d102d66 > src/mem/ruby/system/DMASequencer.cc 9ea24d102d66 > src/mem/ruby/system/MemoryControl.cc 9ea24d102d66 > src/mem/ruby/system/Sequencer.cc 9ea24d102d66 > src/mem/slicc/ast/EnqueueStatementAST.py 9ea24d102d66 > src/mem/slicc/symbols/Type.py 9ea24d102d66 > > Diff: http://reviews.m5sim.org/r/21/diff > > > Testing > ------- > > All regressions pass > > > Thanks, > > Nathan > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
