rcsb / mmtf-cpp

The pure C++ implementation of the MMTF API, decoder and encoder.
MIT License
21 stars 24 forks source link

Fixes for the windows platform #4

Closed frodofine closed 6 years ago

frodofine commented 6 years ago

The windows platform (and the MSVC compiler) require some changes for mmtf-cpp to work properly. This compiler is fairly picky when compared to GCC and Clang, so I needed to change some logic in order to avoid some asserts present in the MSVC version of the standard C++ library.

First, Windows does not define the functions be32toh and the like for swapping Endian modes. I've changed them back to ntohl, which are cross-platform. One must also link to the ws2_32 library to access this functions.

Second, one cannot read a binary file without specifying ios::binary.

Third, operations on vectors of size 0 are not permitted. I've added control code to prevent this from occurring.

Fourth, I've added static_casts to clarify implicit conversion between types.

gtauriello commented 6 years ago

Thanks again for your contributions. Always good to get better cross-platform compatibility... ;-)

The changes are ok and also work successfully on my Linux box.

There is one part I don't like though: the if(encodedDataLength_ == 0) return blocks seem ok for the current use cases but they are not really identical in behavior as running through the code (e.g. the code will actually clear the output vector if size is 0). As such I would prefer if the guard is only put in those operations which Windows dislikes and are guaranteed to do nothing. My guess is that the only part Windows dislikes (and maybe rightly so) are the &output[0] parts in decodeFromBytes_? If that is the case, I would prefer an if (!output.empty()) to guard those calls instead of aborting as early as you did. Or is there some other empty vector operation Windows dislikes? Or any actual operation that we perform on empty vectors?

frodofine commented 6 years ago

That's the vector operation Windows did not like and I agree that guarding that particular operation is more appropriate. I've moved the check.