OutpostUniverse / NetFixServer

0 stars 0 forks source link

Use value initialization instead of `memset` #48

Closed DanRStevens closed 4 years ago

DanRStevens commented 4 years ago

We should prefer value initialization for clearing out structs, rather than memset.

Original using memset:

// Clear counters
memset(&counters, 0, sizeof(counters));

Value initialization:

// Clear counters
counters = {};

References: CppReference: Value initialization (since C++03) Andrzej's C++ blog: Value-initialization with C++


The above may only be possible when there is no default constructor:

Note that if a class defines (implicitly or not) a default constructor, empty braces are not interpreted as zero-element initializer list

Related to the default constructor, is determining if a type is an aggregate type. For older versions of C++ (before C++14), having default field values could make a type non-aggregate. For such types, an implicit default constructor would set those types. This may be relevant here as GameSession has a default field value:

unsigned int flags = 0;

Given that we target C++17, this may not matter. We might want to revisit though, in case it gets in the way. The other side of this is that perhaps value initialization or aggregate initialization would be able to replace the need for the default value.

Brett208 commented 4 years ago

The way I am reading Andrzej's blog post, hew would recommend write GameSession as:

struct GameSession
{
    GUID sessionIdentifier{};
    sockaddr_in addr{};
    time_t time{};
    unsigned int clientRandValue{};
    unsigned int serverRandValue{};
    unsigned int flags{};
    CreateGameInfo createGameInfo{};

    inline bool SocketAddressMatches(const sockaddr_in& socketAddress) const
    {
        return memcmp(&addr, &socketAddress, sizeof(socketAddress)) == 0;
    }
};

This will ensure all members are initialized to default values all the time.

Whether we re-write GameSession this way or not, I agree in changing out memset in GameServer's constructor. I had thought about doing this when rewriting the constructor earlier.

DanRStevens commented 4 years ago

I maybe didn't get quite the same impression from the article.

My thought was, rather than initialize all values all the time, you can instead initialize all values some of the time:

counters = {};

By removing the default initialization from the struct, you can instead choose when values are initialized, (and to what). Empty braces are equivalent to a memset to 0 (roughly, for unpadded structs). A list of values will instead initialize the fields to the given values (and for short lists, any omitted values are initialized to 0).

GameSession gameSession = {sessionIdentifier, addr, time}; // Remaining are zeroed
Brett208 commented 4 years ago

I'm fine with using counter = {}; and closing the issue out.

However, I'm curious what you think of this portion of the post:

Going back to our example with class called value_initialization, it is risky to put built-in types as members in a class, because you may forget to call their value initialization in the constructor(s). Especially, when you add a new member, and the body of the constructor is in another “cpp” file, you may forget to add value-initialization in all constructors. In C++11, you can fix that problem:

struct cpp11_value_initialization
{
  int i {};
  std::mutex m {};
  widget w {};
  int a[9] {};

  // no default constructor required
};

Syntax int i {}; here means, “unless specified otherwise in the constructor, value-initialize member i upon initialization.” Similarly, in “generic context”:

I think the post is saying initializing in this fashion does not interfere with the struct being an aggregate type or receiving specific initialization instructions when the struct is created. I assume you take a small performance hit but allow the values to always be initialized to their defaults. When debugging, it helps me spot uninitialized data if it is all set to default values, although others prefer not setting to default values as it could hide bugs that do not surface with default values.

C++ makes everything complex, even how you initialize values... :|

DanRStevens commented 4 years ago

Always initializing can impact performance, though depends on the optimizer (and optimization level).

Default values can prevent potential crashing problems, though as you hinted, it may do so by hiding the symptoms of using uninitialized values. The compiler can't warn about use of uninitialized values if you are always default initializing (to a potentially wrong value).

I find default initialization makes more sense when the data type is a class. When you're declaring a struct, I prefer not to have default initialization. The reason being that a class may need to maintain some sort of invariant, and so default initialization of a private member field may be necessary to maintain that invariant. For a struct, there should be no invariant, and as all fields are public, the code can change any value to whatever it wants anyway.


I'll make the one line suggested change.

DanRStevens commented 4 years ago

Oh, and you're right that default initialization doesn't cause a struct to be non-aggregate, ... unless you're building to an old standard, such as C++11, in which case default initial values do make the struct non-aggregate (and hence non-POD). Another caveat is that in C++11 having a default value will cause brace initialization to be an error. Those restrictions are lifted in C++14.

Some test code to explore with on Godbolt:

struct A {
  int a;// = 0;
  int b;
};

int main() {
    A a;// = {1, 2};
    return 0;
}

If both assignments are uncommented, and you compile with -std=c++11 (Assuming GCC), the compiler will give an error. It should work if either or both of the two assignments are commented, or if the standard is changed to -std=c++14.