----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/336/#review542 -----------------------------------------------------------
I second Nate: there's fatal() and panic() also take cprintf-style arguments, so there's no point in doing a warn() first followed by fatal() or panic(); the whole message should just be passed to fatal()/panic(). I also don't understand why you kept the ASSERT(0) lines; seems like they should be panics too. See http://m5sim.org/wiki/index.php/Coding_Style#Fatal_vs._panic for the difference between fatal() and panic() (if you haven't already). It's a little hard to tell from just a few lines of context which one is correct, but basically anything that's a programming error rather than a user/configuration error should be panic(), so I'm guessing that some of your fatals should be panics based on that. Also, since the old ERROR_MSG macro called abort(), it's more similar to panic(), so I'd say by default you should use panic unless you know that something reflects a user/configuration error. This last comment is a little unrelated, but I noticed that several of the ERROR_MSG() calls are followed by return statements even though the program terminates. panic() and fatal() are marked as "noreturn" so you should probably be able to eliminate those pointless return statements without incurring any compiler error messages. - Steve On 2010-12-19 12:02:55, Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/336/ > ----------------------------------------------------------- > > (Updated 2010-12-19 12:02:55) > > > Review request for Default. > > > Summary > ------- > > This patch removes the WARN_* and ERROR_* from src/mem/ruby/common/Debug.hh > file. These statements have been replaced with warn(), panic() and fatal() > defined in src/base/misc.hh > > > Diffs > ----- > > src/mem/protocol/RubySlicc_Util.sm 998b217dcae7 > src/mem/ruby/buffers/MessageBuffer.cc 998b217dcae7 > src/cpu/testers/rubytest/RubyTester.cc 998b217dcae7 > src/cpu/testers/rubytest/CheckTable.cc 998b217dcae7 > src/cpu/testers/rubytest/Check.cc 998b217dcae7 > src/mem/ruby/common/Debug.hh 998b217dcae7 > src/mem/ruby/common/NetDest.cc 998b217dcae7 > src/mem/ruby/common/Set.cc 998b217dcae7 > src/mem/ruby/network/Network.cc 998b217dcae7 > src/mem/ruby/network/garnet/fixed-pipeline/GarnetNetwork_d.cc 998b217dcae7 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc > 998b217dcae7 > src/mem/ruby/network/garnet/fixed-pipeline/RoutingUnit_d.cc 998b217dcae7 > src/mem/ruby/network/garnet/fixed-pipeline/SWallocator_d.cc 998b217dcae7 > src/mem/ruby/network/garnet/fixed-pipeline/VCallocator_d.cc 998b217dcae7 > src/mem/ruby/network/garnet/flexible-pipeline/GarnetNetwork.cc 998b217dcae7 > src/mem/ruby/slicc_interface/RubySlicc_ComponentMapping.hh 998b217dcae7 > src/mem/ruby/slicc_interface/RubySlicc_Util.hh 998b217dcae7 > src/mem/ruby/storebuffer/storebuffer.cc 998b217dcae7 > src/mem/ruby/system/CacheMemory.cc 998b217dcae7 > src/mem/ruby/system/PerfectCacheMemory.hh 998b217dcae7 > src/mem/ruby/system/Sequencer.cc 998b217dcae7 > src/mem/ruby/tester/DeterministicDriver.cc 998b217dcae7 > src/mem/ruby/tester/RaceyPseudoThread.cc 998b217dcae7 > src/mem/ruby/tester/test_framework.cc 998b217dcae7 > src/mem/slicc/symbols/StateMachine.py 998b217dcae7 > src/mem/slicc/symbols/Type.py 998b217dcae7 > > Diff: http://reviews.m5sim.org/r/336/diff > > > Testing > ------- > > > Thanks, > > Nilay > >
_______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
