On Thu, Mar 18, 2010 at 4:36 PM, nathan binkert <[email protected]> wrote:
> - You have some more random whitespace changes in there.
> - You seem to have made some changes to code that was commented out.
> Can we just remove that code?
> - Is SparseMemory a Ruby specific thing, or could it be moved to src/mem?
> - Please use M5 style (4 space indent, function name at the beginning
> of a line, etc.) on new files.
What about other higher-level Ruby vs. M5 style things, like the '_t'
suffix on types, the 'm_' prefix on member vars, cprintf vs '<<',
etc.? I can sort of see wanting to keep the code in the Ruby dir
consistent, but I hope that we can work toward a globally consistent
style, and writing new code in M5 style would be an important part of
that, I'd think.
>> + if (m_use_map == false) {
I think we just got done beating this to death in another thread....
>> + for (uint64 i=0;i<m_num_entries;i++) {
Needs more spaces:
for (uint64 i = 0; i < m_num_entries; i++) {
>> -int DirectoryMemory::mapAddressToLocalIdx(PhysAddress address)
>> +uint64 DirectoryMemory::mapAddressToLocalIdx(PhysAddress address)
>> {
>> - int ret = address.getAddress() >> (RubySystem::getBlockSizeBits() +
>> m_num_directories_bits);
>> + uint64 ret = address.getAddress() \
>> + >> static_cast<uint64>(RubySystem::getBlockSizeBits()
>> + + m_num_directories_bits);
>> return ret;
>> }
Is that a backslash in there? For a moment I thought this was Python code...
I don't think that static_cast is needed... according to MSDN, "The
type of the result is the same as the type of the left operand.", so
the right operand can be any integer type.
http://msdn.microsoft.com/en-us/library/336xbhcz.aspx
>> + if (m_use_map) {
>> + if (m_sparseMemory->exist(address)) {
Since I've been beating you up for the explicit bool comparisons, I'll
stop and say how nice these look...
>> + if(m_entries[index] != NULL){
Need a space after "if"... I'll leave the complaining about explicit
NULL comparisons to Nate.
>> + m_map_head = new m5::hash_map<Address, SparseMemEntry_t>;
Can you make a typedef for this? It's pretty awkward...
>> +void SparseMemory::recursivelyRemoveTables(
>> + m5::hash_map<Address, SparseMemEntry_t>* curTable,
>> + int curLevel
>> + )
Assuming a typedef, this should be:
void
SparseMemory::recursivelyRemoveTables(CrazyMapType *curTable, int curLevel)
{
or maybe, if necessary:
void
SparseMemory::recursivelyRemoveTables(CrazyMapType *curTable,
int curLevel)
{
>> + assert(exist(address) == false);
:-) There are several more assertions with explicit '== false 'or '!=
false' comparisons below.
>> + m5::hash_map<Address, SparseMemEntry_t>* tempMap = new
>> m5::hash_map<Address, SparseMemEntry_t>;
Ooh, really need a typedef.
>> +typedef struct SparseMemEntry {
>> + void* entry;
>> +} SparseMemEntry_t;
>> +
>> +typedef struct curNextInfo {
>> + m5::hash_map<Address, SparseMemEntry_t>* curTable;
>> + int level;
>> + int highBit;
>> + int lowBit;
>> +} curNextInfo_t;
The whole typedef struct thing is a C-ism that went out the door in
C++. 'struct' in C++ is just a class with members that are public by
default, so there's no point in declaring them differently. Plus then
you can get rid of the '_t' thing :-).
>> +class SparseMemory {
>> +public:
>> +
>> + // Constructors
>> + SparseMemory(int number_of_bits, int number_of_levels);
>> +
>> + // Destructor
>> + ~SparseMemory();
>> +
>> + // Public Methods
>> +
>> + void printConfig(ostream& out) { }
>> +
>> + bool exist(const Address& address) const;
This is minor, but the code sounds more natural in my head if this
function is named 'exists' rather than 'exist'.
Steve
_______________________________________________
m5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/m5-dev
