Code Review

Started by
11 comments, last by Oberon_Command 8 years, 8 months ago


'rotated' is supposed to be treated as a bool value. I use uint8_t because I wanted it strictly typed for POD purposes so it's more portable when exporting to a file. I try to stay away from bool when working with POD due varying to compiler implementation. bool can vary between platforms/implementations, so I wanted to strictly type something to an unsigned 8-bit value. Also, it doesn't seem like I can XOR bools on some compiler. I use XOR to toggle rotation state of my BinRects. If using a bool, I could toggle simply by myBoolVar = !myBoolVar. Checking > 0 is true, but again, I should just check if(rotated == true) { }. Would a bool still be a better way to go? It'd certainly make it more readable.

If you want to go for portable serialization you should either be using a library(e.g. Boost::Serialization) or manually write the serialization code, because even for POD structures, the memory layout may differ due to alignment and padding and then there's also the issue of endianness.

Also for clarity sake simply write the following instead:

if(rotated) {}
Advertisement


I think it's a pretty good idea to read Scott Meyers's Effective C++.

I've looked at other C++ books recently. I wanted to get one on STL and templates in general. I've heard Effective C++ was a really good one, but wasn't sure which version to get.


Performance in this regards will be increased when you pass stuff like std::vector, std::string which can take a large amount of time to copy. I wouldn't say they exactly increase security, since you can do stuff like this unfortunately:
void takeString(std::string& string)
{
string.clear();
}

std::string* pString = nullptr;

takeString(*pString); // crash inside the function as if you were passing nullptr
but they document intent by telling the user of your function "the value of this parameter is required", while using only pointers technically make it unclear whether or not they have to take a value or can be nullptr.

This is a really good point you've brought up. After reading how references are like pointers that can't be NULL, it looks like I could still pass NULL/invalid pointers into the reference parameter parameter by pointed value, which will result in a segment fault.


f you want to go for portable serialization you should either be using a library(e.g. Boost::Serialization) or manually write the serialization code, because even for POD structures, the memory layout may differ due to alignment and padding and then there's also the issue of endianness.

This is another thing I keep going back and forth on. When I started learning C/C++, there were cases where the PSP homebrew community would store their data as POD if they could afford to in an effort to perform less calls to fread() and fwrite(). Again, padding and endianess between environments is a caveat.

As far as this convention goes:


if(rotated) { }

I actually prefer it that way for bools, pointers and integer values where zero acts as a special use-case versus a non-zero value. The only reason I wrote that as if(rotated > 0) { } is because I thought that'd be a more accepted convention. I'll post more often when I have questions regarding code conventions.


void takeString(std::string& string)
{
    string.clear();
}

std::string* pString = nullptr;

takeString(*pString); // crash inside the function as if you were passing nullptr


I believe the actual problem there is the fact that you're explicitly dereferencing a nullptr to pass it to a function that expects a reference. A better example would be this:


struct foo 
{
   std::unique_ptr<int> ptr;

   const int& getRef() const
   {
       return *(ptr.get());
   }
};

foo bar = { std::make_unique<int>(5) };
const int& bad = bar.getRef();
printf("%d", bad); // prints '5'

// ...

bar.ptr.release();
printf("%d", bad); // bad is now pointing at released memory - might crash, might keep running(!)

This topic is closed to new replies.

Advertisement