Improve GPGNet parser

[muellni]

To implement suggestions made by ckitching:

https://github.com/FAForever/ice-adapter/blob/master/GPGNetMessage.cpp#L15 This sort of casting is a violation of TBAA, and is undefined behaviour. See the section of the manual here titled "type aliasing": https://en.cppreference.com/w/cpp/language/reinterpret_cast If you want to do this kind of "unsafe" casting of bits between types, you must use a memcpy to do it (and then the copy-propagator will eat the copy,of course). In another project of mine I have this utility function for that purpose:

 * Perform a type-unsafe reinterpret cast, avoiding undefined behaviour.
 *
 * Due to TBAA, you can't just do a pointer cast and dereference it. This function nicely abstracts the necessary
 * memcpy (which the optimiser will eat anyway).
 *
 * See [the manual](http://en.cppreference.com/w/cpp/language/reinterpret_cast)
 */
template<typename T, typename Q>
T reinterpretAs(const Q& value) {
    static_assert(sizeof(T) == sizeof(Q), "Unsafe reinterpretation between types of a different size. This is not C!");
    static_assert(std::is_default_constructible<T>(), "Targets for unsafe reinterpretation must be default-constructible");

    T out;
    memcpy(&out, &value, sizeof(Q));
    return out;
};

If you don't, the compiler may destroy your program. The TBAA rules allow the compiler to assume that pointers to things of different types never point to the same underlying memory (except in the case of inheritance relations). A simple example of how this might break a program is if you write through a T* which you got from an undefined cast from a Q*, the compiler will assume that this cannot change the value of any variables of type Q. It would then be allowed to reorder memory operations in a way that hides your write, or even optimise it out completely (if the Q is never read except through the illegally-obtained T*, then the Q is dead as far as the optimiser is concerned, and may be removed). (edited) Although in this specific case you might find std::to_string is actually what you want. On another note, this toBinary function's repeated use of string::append is going to lead to an avalanche of memory allocations as it repeatedly grows the strings. If this is hot code, you're gonna want to fix that. Another invalid cast here: https://github.com/FAForever/ice-adapter/blob/master/GPGNetMessage.cpp#L111This one is undefined behaviour for an additional reason: it relies on the (undefined) property that std::string::iterator is implemented using an integer here. The defined way to compute what you want would be to use iterator subtraction. I'd advise you guys to run this program through UndefinedBehaviourSanitiser: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html Honestly, it looks like virtually every reinterpret_cast in your program is a TBAA violation :smile:

So, just another thing: the reinterpretAs() routine I gave above can help with this sort of casting, but even this is not enough in cases where the underlying layout is also undefined (such as for iterators) Oh yeah, other thing: storing binary data in strings like this is deeply questionable anyway. :

https://github.com/FAForever/ice-adapter/blob/master/GPGNetMessage.cpp#L41 Unnecessary conversion between std::string and C-string (std::string::apend(std::string&) exists, so you can just append the string directly) https://github.com/FAForever/ice-adapter/blob/master/GPGNetMessage.h#L18 This doccomment's parameter signature is wrong. I suggest enabling -Wdocumentation in your compiler to catch problems like this https://github.com/FAForever/ice-adapter/blob/master/cxxopts.hpp Random thing: DOCOPT IS AWESOME, you may want to consider it: https://github.com/docopt/docopt.cppObviously not that important. https://github.com/FAForever/ice-adapter/blob/master/GPGNetServer.cpp#L83 Replace with range-for.