vrpn / vrpn

Virtual Reality Peripheral Network - Official GitHub Repository
https://github.com/vrpn/vrpn/wiki
396 stars 216 forks source link

vrpn_big_endian #117

Open rpavlik opened 9 years ago

rpavlik commented 9 years ago

The current approach uses a static variable declared in a header file, so we get a ton of these warnings, one in nearly every source file:

vrpn_Shared.h:210:19: warning: 'vrpn_big_endian' defined but not used [-Wunused-variable]
 static const bool vrpn_big_endian = (vrpn_char_data_for_endian_test[0] != 1);

I'm not sure the current approach is even necessarily safe (think we should just be doing a reinterpret_cast<const char *> since aliased access through a character type is explicitly permitted by the standard), but furthermore, on most platforms there's a preprocessor definition that can provide a more complete endianness answer at preprocessor time (cf. https://github.com/boostorg/predef/blob/develop/include/boost/predef/other/endian.h ), not just at optimize time (I'm hoping that variable gets optimized out...)

Of course, the hairy part is, this is in a public header, and I don't know who is all consuming it. If nothing else, it should be moved to a separate header to be included if desired, and preferably it should be removed entirely except in cases where it's a really old/obscure machine and so the typical endianness definitions won't suffice.

russell-taylor commented 9 years ago

That's a leftover from the really old days when there was not a better way to find out. It was intended to be used in the internal code, but you're right that others may have latched on to it. I'm really opposed to pulling in boost just to get at endian-ness, but am certainly okay with replacing it with an extern bool reference and then moving the definition into the .C file. I think it was in the header originally to make it possible for compilers to inline it. It could also be replaced by #define to call a function that returns a bool that could be inlined.

rpavlik commented 9 years ago

Yeah, wasn't suggesting pulling in boost, the defines are not that complicated, they just have them handy there (and license-compatible). The define to call a function seems like the best way to avoid breaking existing code - hadn't thought of that one (I try to avoid the preprocessor)