Nothing seriously wrong in here. One unnecessary cast, but otherwise looks good.
void Memory::writeLong(const long address, const long value)
{
if (address < start || address + sizeof(long) > start + memorySize) {
cout << "Memory::readLong out of range" << endl;
throw "Memory class range error";
}
uint8_t *valRep = (uint8_t *) &value;
for (int i = 0; i < sizeof(long); i++)
{
contents[address + i] = *((uint8_t *)(valRep + i));// don't need cast
}
}
Couple bits of nastiness in the read:
long Memory::readLong(const long address)
{
long retVal = 0;
if (address < start || address + sizeof(long) > start + memorySize) {
cout << "Memory::readLong out of range" << endl;
throw "Memory class range error";
}
uint8_t in[8]; // should be uint8_t in[sizeof(retVal)];
// why sizeof(retVal) and not sizeof(long)
// someone could cut and paste to make the different typed
// versions and change only retVal. Might as well lock them
for (int i = 0; i < sizeof(long); i++) //nag nag, signed unsigned mismatch
{
try {
in[i] =(uint8_t) contents.at(address + i) << (i * 8);
// in[i] is 8 bits. << (i * 8) ensures all but the first byte are shifted
// out of byte range
}
catch (const out_of_range& err)
{
contents[address] = 0;
}
}
memcpy(&retVal, in, 8); // overruns retVal if retval is 32 bits. Use
// sizeof(retVal) again.
return retVal;
}
So this can't possibly work or generate the results OP reported. My guess is OP had something that almost worked and then started flailing around trying to fix it and made things worse.
How I'd do it:
Edit: Turns out type punning with unions is illegal. Sucks, because I think it's safer and more easily read than the pointer version. Probably the only reason the pointer version is not illegal is the huge volume of legitimate cases for casting to byte.
I also moved the try/catch block outside the reassembly loop because once you read outside memory, the recovery's pretty much toast.
#include <iostream>
#include <map>
std::map<long, uint8_t> contents;
void writeLong(const size_t address, const long value)
{
/* not needed for test case
if (address < start || address + sizeof(long) > start + memorySize) {
cout << "Memory::readLong out of range" << endl;
throw "Memory class range error";
}
*/
uint8_t *valRep = (uint8_t *) &value;
for (size_t i = 0; i < sizeof(long); i++)
{
contents[address + i] = *(valRep + i);
}
}
long readLong(const size_t address)
{
// Verbotten!
// union
// {
// long val;
// uint8_t arr[sizeof(val)];
// }retVal; // Ahhhrrrrrr! Here be endian nightmare!
long retVal;
uint8_t *valRep = (uint8_t *) &retVal;
// uglier, but legal. Consider using the no-punning version below
/* not needed for test case
if (address < start || address + sizeof(long) > start + memorySize) {
cout << "Memory::readLong out of range" << endl;
throw "Memory class range error";
}
*/
// uint8_t in[sizeof(long)]; replaced by evil union abuse above
try
{
for (size_t i = 0; i < sizeof(long); i++)
{
// Not legal
// retVal.arr[i] = contents.at(address + i) ;
*(valRep + i) = contents.at(address + i);
}
}
catch (const std::out_of_range& err)
{
retVal = 0;
}
// memcpy(&retVal, in, sizeof(long)); replaced by evil union abuse above
// Not legal
// return retVal.val;
return retVal;
}
void test(long val)
{
writeLong(0, val);
std::cout << "In " << val << " out " << readLong(0) << std::endl;
}
int main()
{
test(-4);
test(-8);
test(-5);
test(-1);
test(-1224);
test(0);
test(1);
test(0x7fffffff);
test(0x80000000);
}
Output:
In -4 out -4
In -8 out -8
In -5 out -5
In -1 out -1
In -1224 out -1224
In 0 out 0
In 1 out 1
In 2147483647 out 2147483647
In -2147483648 out -2147483648
I will miss my friend the type punning union, but there are punning-free versions.
void writeLong(size_t address, long value)
{
// not needed for test case
// if (address < start || address + sizeof(long) > start + memorySize) {
// cout << "Memory::readLong out of range" << endl;
// throw "Memory class range error";
// }
for (size_t i = 0; i < sizeof(long); i++)
{ // note this only preserves the proper memory layout for little endian systems
contents[address + i] = (uint8_t)(value &0xFF);
value >>= 8;
}
}
long readLong(size_t address)
{
long retVal;
// not needed for test case
// if (address < start || address + sizeof(long) > start + memorySize) {
// cout << "Memory::readLong out of range" << endl;
// throw "Memory class range error";
// }
try
{
address += sizeof(long) - 1;
retVal = contents.at(address--);
for (size_t i = 0; i < sizeof(long) - 1; i++)
{ // this only works if the little endian memory layout is preserved
retVal <<= 8;
retVal += contents.at(address--);
}
}
catch (const std::out_of_range& err)
{
retVal = 0;
}
return retVal;
}
