Review Board 2.0.15


ruby: get rid of Vector and use STL

Review Request #22 - Created June 2, 2010 and submitted

Information
Nathan Binkert
gem5
Reviewers
Ruby 
ruby: get rid of Vector and use STL
add a couple of helper functions to base for deleteing all pointers in
a container and outputting containers to a stream
All regressions pass
Ship it!
Nilay Vaish
Posted (June 7, 2010, 1:11 p.m.)
1. Should we use resize() or should we use reserve() for setting the capacity of a vector? Especially during initialization, reserve() might be faster since it does not initialize the allocated memory where as resize() carries out some initilization of the newly allocated entries. There are multiple instances were the vector is first resized and then the entires are initialized to some value, thus redoing the work done by resize(). There are instances where we may not need the initialization being carried out. In those cases we can shift to reserve() and then use push_back() instead of [].

2. In src/mem/gems_common/Map.hh :: keys(), values() -- why not keep using the for loop on the iterator? This is probably inconsequential since Nate's later patch for Map does away with this file.

3. I think mem/ruby/common/DataBlock.hh does not need to include <vector>. vector is used neither in DataBlock.hh nor in DataBlock.cc.

4. No need to call clear() in src/mem/ruby/common/NetDest.cc :: getAllDest(). In constructors for several classes, clear() function is being called on the vector data members of the class. Seems need less to me unless an object of the class would be reinitialized.

5. Do we need to do resize() in network/simple/Switch.cc::Switch()?

6. In profiler/CacheProfiler.cc, should we move m_requestTypeVec_ptr to vector from vector*?
  1. Ignore the ship it tag above. I was testing what happens if I check the box provided. Is it a vote in favor of the patch?
  2. 1) It is generally a better practice to try to make changes more self contained.  Switching from resize() to reserve() would in my opinion be doing two different things in the same patch.  I'm fine with you doing this in a separate patch.
2) I guess I could have used reserve and push_back.  It's not worth fixing though since as you say, I delete the file anyway.
3) Fixed
4) This is an unrelated change.  Should be a different changeset.
5) No.  Fixed.
6) Yes. Done

Ship It means that you're happy.
  3. This patch also looks set for shipping.
Brad Beckmann
Posted (June 21, 2010, 8:53 a.m.)
Overall, this patch looks good.  I just have a couple questions about it:

1.  It appears that the stl_helpers class allows us to overload the operator<< for Ruby style printing.  That is great.  Thanks for maintaining this feature as we move to stl.  So have you looked at the print out for Ruby objects such as NetDest?  Are the print outs for these functions equivalent before and after your changes?

2.  Why does DeterministicDriver use the std namespace? 
  1. 1. I thought so, but I guess we should double check at some point.

2. Because std::vector is used without the std:: and I nuked the "using namespace std;" that was in a header somewhere.