Fails to build with GCC 4.4.3
Posted: 15 Aug 2011, 13:12
Chris wrote: Since the -Werror flag was added, OpenMW fails to build because of warnings-turned-errors. In particular, I get these:
components/nif/nif_file.cpp:75: error: â??râ?? may be used uninitialized in this function
components/nif/data.hpp:489: error: â??sizeâ?? may be used uninitialized in this function
These two can probably be fixed by marking the various fail() functions with __attribute__((noreturn)), which tells GCC the function won't return if it's called (thus there's no path that could leave the variables uniniitalized before use). Otherwise you could just initialize them to NULL/0.
However, there's further warnings-turned-errors about dereferencing type-punned pointers (basically, doing something like *(float*)&someint), in various places. By default, compilers are allowed to optimize with the assumption that two different, incompatible pointer types won't refer to the same memory, so changing a float pointer won't modify any int pointers, and vice-versa. The fix for this is to use a union:Just need to make sure the two types are the same size.Code: Select all
union { int someint; float somefloat; }; someint = intvalue; // somefloat now has the same byte values of someint
Zini wrote: Both error messages you listed simply state, that there is an uninitialized local variable, which indeed is not a good idea.
Instead of
it should beCode: Select all
int size;
Code: Select all
int size = 0;The full error message please.However, there's further warnings-turned-errors about dereferencing type-punned pointers (basically, doing something like *(float*)&someint), in various places. By default, compilers are allowed to optimize with the assumption that two different, incompatible pointer types won't refer to the same memory, so changing a float pointer won't modify any int pointers, and vice-versa.
btw. I am not a big fan of -Werror. Sometimes the warning messages on gcc are a bit hard to get under control and turning all of them into errors only serves to make coding harder for us.
Chris wrote:Though if you follow the code, the only path that it remains uniniitalized in is a call to fail(), which won't return for the variable to be used (so the variable won't be used uninitialized, but GCC thinks it can because it thinks fail() can return). A patch like this fixes the warning/error (and should improve compiler optimizations):Zini wrote:Both error messages you listed simply state, that there is an uninitialized local variable, which indeed is not a good idea.
Instead of
it should beCode: Select all
int size;
Code: Select all
int size = 0;Though you'd probably want to put that under a macro, so MSVC can specify the appropriate __declspec (whatever it may be).Code: Select all
diff --git a/components/nif/nif_file.hpp b/components/nif/nif_file.hpp index ebf1fe4..e00bb67 100644 --- a/components/nif/nif_file.hpp +++ b/components/nif/nif_file.hpp @@ -65,7 +65,7 @@ class NIFFile public: /// Used for error handling - void fail(const std::string &msg) + void __attribute__((noreturn)) fail(const std::string &msg) { std::string err = "NIFFile Error: " + msg; err += "\nFile: " + filename;
Not much of a fan of it myself, but leaving warnings is dangerous and "unprofessional". Even when they're harmless, they're not nice to leave in as they can turn into red herrings when a bug pops up.The full error message please.However, there's further warnings-turned-errors about dereferencing type-punned pointers (basically, doing something like *(float*)&someint), in various places. By default, compilers are allowed to optimize with the assumption that two different, incompatible pointer types won't refer to the same memory, so changing a float pointer won't modify any int pointers, and vice-versa.
btw. I am not a big fan of -Werror. Sometimes the warning messages on gcc are a bit hard to get under control and turning all of them into errors only serves to make coding harder for us.
As for the messages, there's first these ones:
components/esm/esm_reader.hpp:154: error: dereferencing type-punned pointer will break strict-aliasing rules
components/esm/loadlocks.hpp:44: error: dereferencing type-punned pointer will break strict-aliasing rules
components/esm/loadlocks.hpp:45: error: dereferencing type-punned pointer will break strict-aliasing rules
If I clear them away, I get a mess from Boost:(the last three lines are the tell-tale ones). With that, I just kinda gave up because I don't see specifically what the problem is or how to fix it.Code: Select all
[ 73%] Building CXX object apps/openmw/CMakeFiles/openmw.dir/mwinput/inputmanager.cpp.o cc1plus: warnings being treated as errors In file included from /usr/include/boost-1_41/boost/function/detail/prologue.hpp:17, from /usr/include/boost-1_41/boost/function.hpp:24, from /home/kitty/openmw/libs/openengine/input/func_binder.hpp:6, from /home/kitty/openmw/libs/openengine/input/dispatcher.hpp:5, from /home/kitty/openmw/apps/openmw/mwinput/inputmanager.cpp:3: /usr/include/boost-1_41/boost/function/function_base.hpp: In static member function â??static void boost::detail::function::functor_manager_common<Functor>::manage_small(const boost::detail::function::function_buffer&, boost::detail::function::function_buffer&, boost::detail::function::functor_manager_operation_type) [with Functor = boost::_bi::bind_t<void, boost::_mfi::mf0<void, MWInput::InputImpl>, boost::_bi::list1<boost::_bi::value<MWInput::InputImpl*> > >]â??: /usr/include/boost-1_41/boost/function/function_base.hpp:358: instantiated from â??static void boost::detail::function::functor_manager<Functor>::manager(const boost::detail::function::function_buffer&, boost::detail::function::function_buffer&, boost::detail::function::functor_manager_operation_type, mpl_::true_) [with Functor = boost::_bi::bind_t<void, boost::_mfi::mf0<void, MWInput::InputImpl>, boost::_bi::list1<boost::_bi::value<MWInput::InputImpl*> > >]â?? /usr/include/boost-1_41/boost/function/function_base.hpp:404: instantiated from â??static void boost::detail::function::functor_manager<Functor>::manager(const boost::detail::function::function_buffer&, boost::detail::function::function_buffer&, boost::detail::function::functor_manager_operation_type, boost::detail::function::function_obj_tag) [with Functor = boost::_bi::bind_t<void, boost::_mfi::mf0<void, MWInput::InputImpl>, boost::_bi::list1<boost::_bi::value<MWInput::InputImpl*> > >]â?? /usr/include/boost-1_41/boost/function/function_base.hpp:432: instantiated from â??static void boost::detail::function::functor_manager<Functor>::manage(const boost::detail::function::function_buffer&, boost::detail::function::function_buffer&, boost::detail::function::functor_manager_operation_type) [with Functor = boost::_bi::bind_t<void, boost::_mfi::mf0<void, MWInput::InputImpl>, boost::_bi::list1<boost::_bi::value<MWInput::InputImpl*> > >]â?? /usr/include/boost-1_41/boost/function/function_template.hpp:913: instantiated from â??void boost::function2<R, T1, T2>::assign_to(Functor) [with Functor = boost::_bi::bind_t<void, boost::_mfi::mf0<void, MWInput::InputImpl>, boost::_bi::list1<boost::_bi::value<MWInput::InputImpl*> > >, R = void, T0 = int, T1 = const void*]â?? /usr/include/boost-1_41/boost/function/function_template.hpp:722: instantiated from â??boost::function2<R, T1, T2>::function2(Functor, typename boost::enable_if_c<boost::type_traits::ice_not::value, int>::type) [with Functor = boost::_bi::bind_t<void, boost::_mfi::mf0<void, MWInput::InputImpl>, boost::_bi::list1<boost::_bi::value<MWInput::InputImpl*> > >, R = void, T0 = int, T1 = const void*]â?? /usr/include/boost-1_41/boost/function/function_template.hpp:1064: instantiated from â??boost::function<R(T0, T1)>::function(Functor, typename boost::enable_if_c<boost::type_traits::ice_not::value, int>::type) [with Functor = boost::_bi::bind_t<void, boost::_mfi::mf0<void, MWInput::InputImpl>, boost::_bi::list1<boost::_bi::value<MWInput::InputImpl*> > >, R = void, T0 = int, T1 = const void*]â?? /home/kitty/openmw/apps/openmw/mwinput/inputmanager.cpp:178: instantiated from here /usr/include/boost-1_41/boost/function/function_base.hpp:319: error: dereferencing type-punned pointer will break strict-aliasing rules /usr/include/boost-1_41/boost/function/function_base.hpp:323: error: dereferencing type-punned pointer will break strict-aliasing rules
Chris wrote: Here's essentially what I did to get by the errors before Boost:I don't like the memcpy's, but what ya gonna do?Code: Select all
diff --git a/components/esm/esm_reader.hpp b/components/esm/esm_reader.hpp index 64e0861..e117dca 100644 --- a/components/esm/esm_reader.hpp +++ b/components/esm/esm_reader.hpp @@ -86,7 +86,10 @@ struct HEDRstruct versions are 1.2 and 1.3. These correspond to: 1.2 = 0x3f99999a and 1.3 = 0x3fa66666 */ - int version; + union { + int version; + float versionf; + }; int type; // 0=esp, 1=esm, 32=ess NAME32 author; // Author's name NAME256 desc; // File description @@ -151,7 +154,7 @@ public: *************************************************************************/ int getVer() { return c.header.version; } - float getFVer() { return *((float*)&c.header.version); } + float getFVer() { return c.header.versionf; } int getSpecial() { return spf; } const std::string getAuthor() { return c.header.author.toString(); } const std::string getDesc() { return c.header.desc.toString(); } diff --git a/components/esm/loadlocks.hpp b/components/esm/loadlocks.hpp index 3b7913e..cc31b29 100644 --- a/components/esm/loadlocks.hpp +++ b/components/esm/loadlocks.hpp @@ -41,9 +41,9 @@ struct Tool if(n == "RIDT") { // Swap t.data.quality and t.data.uses for repair items (sigh) - float tmp = *((float*)&data.uses); - data.uses = *((int*)&data.quality); - data.quality = tmp; + int tmp = data.uses; + memcpy(&data.uses, &data.quality, 4); + memcpy(&data.quality, &tmp, 4); } script = esm.getHNOString("SCRI"); diff --git a/components/nif/nif_file.hpp b/components/nif/nif_file.hpp index ebf1fe4..59e94e5 100644 --- a/components/nif/nif_file.hpp +++ b/components/nif/nif_file.hpp @@ -36,6 +36,12 @@ #include "record.hpp" #include "nif_types.hpp" +#ifdef __GNUC__ +#define NORETURN __attribute__((noreturn)) +#else +#define NORETURN +#endif + using namespace Mangle::Stream; namespace Nif @@ -65,7 +71,7 @@ class NIFFile public: /// Used for error handling - void fail(const std::string &msg) + void NORETURN fail(const std::string &msg) { std::string err = "NIFFile Error: " + msg; err += "\nFile: " + filename;
Zini wrote:Uhm ... no? Unless profiling shows very clearly, that we have a bottleneck here, that can be avoided by not writingvoid __attribute__((noreturn)) fail(const std::string &msg)
(very unlikely btw.) code clarity is much more important and this line makes the code substantially harder to read.Code: Select all
int size = 0;
Looks like boost might be not up to the much stricter standard conformity of later gcc versions. Or it is a well hidden OpenMW problem. I don't know this part of OpenMW well enough.
The components/esm/esm_reader.hpp code looks odd. Would have to look into the esm format documentation to make sense of it.
For loadlocks a union seems to bit of an overkill (and it complicates the usage of ESM::Tool. I would just declare a second data structure with a switched parameter sequence:
This makes the fix entirely local and no code, that uses Tool has to be changed.Code: Select all
if(n == "RIDT") { Data2 data2; // argument order swapped esm.getHT (data2, 16); assign contents of data2 to data }
But honestly I think -Werror has to go. While it is not nice to have warnings, it is sometimes unavoidable (at least temporarily). This shouldn't be a show-stopper.
Chris wrote:No idea what you mean. Adding a noreturn attribute (especially if it's hidden behind a macro like in the patch) doesn't hinder readability at all. In fact, it makes it quite clear that the calling function will not be executing anymore after a call to fail(). It also guarantees all non-failure paths are properly setting variables (whereas if you initialize it to 0, a bug could slip through that erroneously leaves it at 0, and be harder to track down later).Zini wrote:Uhm ... no? Unless profiling shows very clearly, that we have a bottleneck here, that can be avoided by not writingvoid __attribute__((noreturn)) fail(const std::string &msg)
(very unlikely btw.) code clarity is much more important and this line makes the code substantially harder to read.Code: Select all
int size = 0;
The patch above fixes it by using two memcpy's and a tmp var.The components/esm/esm_reader.hpp code looks odd. Would have to look into the esm format documentation to make sense of it.
For loadlocks a union seems to bit of an overkill (and it complicates the usage of ESM::Tool.
Zini wrote: I don't like the memcpy either.I think my proposed solution is cleaner.
Regarding the NORETURN: Please, don't! See my post above for reasons why not.
Regarding the esm_reader code: I am not sure what the purpose of the whole construct is (in the original code, before you modified it). In the documentation that I have about the esm format there is no hint that the version number can be interpreted as an integer as well as a float. But maybe I am missing something.
Yes it does. Badly. Because it basically redefines a part of the language syntax. Hiding it behind a macro does not improve anything at all (except portability). It is still an unknown construct to a coder, that doesn't know about the macro.No idea what you mean. Adding a noreturn attribute (especially if it's hidden behind a macro like in the patch) doesn't hinder readability at all
Chris wrote:Not sure about that. You'd have to declare a second (almost identical) type, and an operator= to copy the values from it to the "real" Data struct.Zini wrote:I don't like the memcpy either.
IMO, the best way to handle it would be to not load whole structs from file, but instead handle them one member at a time. That way you don't have to worry about struct sizes perfectly matching, padding, type sizes, or endianess (I'm sure there are people who would love to see OpenMW run on portables or embedded systems; PS3 or Wii homebrew? Android?). And in a case like this, let's you check the subname and invert the last two read calls as appropriate.
Premature optimization is the root of all evil...
The way it looks to me, the version number is read as an int so that it can be properly checked against a hex value. Comparing two floats for equality is a huge no-no, but you can check the 32-bit hex value of the float instead if it's interpreted as an 32-bit int.Regarding the esm_reader code: I am not sure what the purpose of the whole construct is (in the original code, before you modified it). In the documentation that I have about the esm format there is no hint that the version number can be interpreted as an integer as well as a float. But maybe I am missing something.
I don't think you're giving enough credit. The macro name is succinct, and describes exactly what it is. It's no worse than putting WINAPI, stdcall, or cdecl in the same spot (and if you want to talk about clarity for the coder..) In all honesty, I wouldn't doubt that MSVC uses __declspec(noreturn) for the same exact purpose. Standard C++ doesn't define it, but if it's a completely normal compiler feature, it should be taken advantage of if it's available (IMO). It helps the compiler understand what you intend the code to do, which helps with static analysis, which helps point out potential problems and to shut up about non-issues.Yes it does. Badly. Because it basically redefines a part of the language syntax. Hiding it behind a macro does not improve anything at all (except portability). It is still an unknown construct to a coder, that doesn't know about the macro.No idea what you mean. Adding a noreturn attribute (especially if it's hidden behind a macro like in the patch) doesn't hinder readability at all
Zini wrote:This is a common myth. But it is completely nonsense. You can compare floats with the equality operator just fine. You only get into trouble, when rounding is involved.Comparing two floats for equality is a huge no-no
This I fully agree with.IMO, the best way to handle it would be to not load whole structs from file, but instead handle them one member at a time. That way you don't have to worry about struct sizes perfectly matching, padding, type sizes, or endianess (I'm sure there are people who would love to see OpenMW run on portables or embedded systems; PS3 or Wii homebrew? Android?).
It is exactly as bad as the WINAPI crap.I don't think you're giving enough credit. The macro name is succinct, and describes exactly what it is. It's no worse than putting WINAPI, stdcall, or cdecl in the same spot (and if you want to talk about clarity for the coder..)
A non-standard compiler feature. Exactly. Limited to some compilers. And not necessarily known to your average C++ coder. NORETURN my sound clear to you, but if you don't know about this feature, it will still look like a syntactical oddity. A responsible coder will starting investigating its functionality when he encounters it and thus wastes time and focus on something of comparably little benefit.Standard C++ doesn't define it, but if it's a completely normal compiler feature,
More than enough reason to not use it.
Well, then we have to agree to not to agree. My position is to avoid it at all cost unless you have no other option.it should be taken advantage of if it's available (IMO).
nicolay wrote:Thanks for the report. Strange, I'm using 4.4.3 too and I don't get any or these warnings / errors.Chris wrote:Since the -Werror flag was added, OpenMW fails to build because of warnings-turned-errors. In particular, I get these:
components/nif/nif_file.cpp:75: error: â??râ?? may be used uninitialized in this function
components/nif/data.hpp:489: error: â??sizeâ?? may be used uninitialized in this function
I picked the easy solution and initialized both vars. I also disabled -Werror. For me the warnings were few and far between so I didn't see any harm in adding it (especially since I frequently missed the warnings on long compiles.) But it looks like people is getting warnings I'm not, so I'll take it out again.
Ok. But I personally don't see any reason to go through elaborate steps to change this, it's completely valid C/C++ code.However, there's further warnings-turned-errors about dereferencing type-punned pointers (basically, doing something like *(float*)&someint), in various places. By default, compilers are allowed to optimize with the assumption that two different, incompatible pointer types won't refer to the same memory, so changing a float pointer won't modify any int pointers, and vice-versa.
Chris wrote:Which is the problem.. you can't be sure how the FPU will round, as it internally works in a much higher bit depth than floats (or even doubles). Take this code for instance:Zini wrote:This is a common myth. But it is completely nonsense. You can compare floats with the equality operator just fine. You only get into trouble, when rounding is involved.In this case, it's entirely possible for CheckVer to return false because 1.2 cannot be represented exactly by IEEE floating-point. The 1.2 param will be rounded to fit into 32-bit storage, while the 1.2 used to check against can be loaded in the FPU's full bit depth (basically, the float param would be along the lines of 1.20000001, while the explicit value to check against would be 1.200000000000001, leading to inequality).Code: Select all
bool CheckVer(float ver) { if(ver == 1.2) return true; return false; } ... if(CheckVer(1.2)) ...
Additionally, the FPU's rounding mode and depth can change, so if one float was rounded one way, and another float was rounded another, you would get inequality for a value that is logically the same. Floating-point is horrible if you need exactness.
You can say the same thing about any standard construct the coder may not be familiar with.A non-standard compiler feature. Exactly. Limited to some compilers. And not necessarily known to your average C++ coder. NORETURN my sound clear to you, but if you don't know about this feature, it will still look like a syntactical oddity. A responsible coder will starting investigating its functionality when he encounters it and thus wastes time and focus on something of comparably little benefit.
Is it really a problem to use an optional feature because someone may not immediately understand it, and possibly take a few minutes learning about it? If anything, that means people should use it more often, so it's not so obscure.. I don't think I can quantify all I've discovered and learned because I've looked up information on things other projects have done. Encouraging people to learn is hardly a bad thing; it's one of the bigger benefits of open source.
I would agree with you if it was a feature that's not widely supported and was required to make the code work properly, but that's not the case here.
There are some versions of GCC that fail to find potentially uninitialized variables when branching is involved. Perhaps my system has that bug fix back ported and yours doesn't? The missing type-punned pointer warnings are an odd case, though. Might be one of the pkg-config files adding extra command line arguments to enable/disable certain warnings..nicolay wrote:Thanks for the report. Strange, I'm using 4.4.3 too and I don't get any or these warnings / errors.Chris wrote:Since the -Werror flag was added, OpenMW fails to build because of warnings-turned-errors. In particular, I get these:
components/nif/nif_file.cpp:75: error: â??râ?? may be used uninitialized in this function
components/nif/data.hpp:489: error: â??sizeâ?? may be used uninitialized in this function
I think the point is that it isn't 100% valid, and GCC is warning you about that. It's valid in that the language allows you to do it, but behavior is undefined because of C/C++'s rules on aliased pointers. If you just ignore the warning, you're potentially letting in heisenbugs (since debug can optimize differently, if at all, and change the behavior of the construct).. if you disable that optimization then you disallow the compiler from generating the best code it can (and rely on a specific compiler feature for a proper build, as other compilers may also apply that optimization with no switch to disable it).Ok. But I personally don't see any reason to go through elaborate steps to change this, it's completely valid C/C++ code.
Zini wrote:You are mixing floats and doubles in your example. That obviously can go wrong. There should be no such problems if you force all values into a float.In this case, it's entirely possible for CheckVer to return false because 1.2 cannot be represented exactly by IEEE floating-point. The 1.2 param will be rounded to fit into 32-bit storage, while the 1.2 used to check against can be loaded in the FPU's full bit depth (basically, the float param would be along the lines of 1.20000001, while the explicit value to check against would be 1.200000000000001, leading to inequality).
Chris wrote:Point still remains though, it's dangerous. If obscure run-time errors can be caused solely by the difference between 1.2 and 1.2f, is it worth the risk? And it still doesn't solve the problem of changes in FPU rounding mode.Zini wrote:You are mixing floats and doubles in your example. That obviously can go wrong. There should be no such problems if you force all values into a float.
Zini wrote:What problem? 1.2f is 1.2f. There is no FPU rounding involved in generating the float value for this numeric literal. It is the compiler, that is doing the transformation source code string -> float value.And it still doesn't solve the problem of changes in FPU rounding mode.
Then the float value is passed to the FPU at some point, which may widen it into an internal format. But because this format is wider it can represent the original (rounded by compiler) value exactly, so no additional rounding happens.
Yes and no. Its no more obscure than anything else regarding floating point numbers. Nothing wrong with doing a plain comparison for equality, when you need one.Point still remains though, it's dangerous. If obscure run-time errors can be caused solely by the difference between 1.2 and 1.2f, is it worth the risk?
But on the other hand I would have avoided floating point numbers in the first place (unless I absolutely need them). Coming back to the original code, using a floating point number for a version number is rather bogus. An int would have served better here.
Chris wrote:Some rounding is still occuring, whether it's done at run-time or compile-time. Can you be sure it'll round as you need it to?Zini wrote:What problem? 1.2f is 1.2f. There is no FPU rounding involved in generating the float value for this numeric literal. It is the compiler, that is doing the transformation source code string -> float value.And it still doesn't solve the problem of changes in FPU rounding mode.
I wouldn't say that. It would be rather easy for a patch to accidentally drop the f, and since you won't get any compile warnings about it, you have to be extra dilligent when searching for the bug. If you weren't sure what to look for, you you notice a lost f and realize it to be the source of a bug?Yes and no. Its no more obscure than anything else regarding floating point numbers.Point still remains though, it's dangerous. If obscure run-time errors can be caused solely by the difference between 1.2 and 1.2f, is it worth the risk?
Quite true, and there shouldn't be any reason OpenMW needs to access it as a float. It looks like the code already treats the version as an int/enum, with VER_12 = 0x3f99999a and VER_13 = 0x3fa66666, so it can infer "1.2" and "1.3" from those.But on the other hand I would have avoided floating point numbers in the first place (unless I absolutely need them). Coming back to the original code, using a floating point number for a version number is rather bogus. An int would have served better here.
EDIT:
Indeed, this patch works:Code: Select all
diff --git a/apps/esmtool/esmtool.cpp b/apps/esmtool/esmtool.cpp index fe067d8..96ccd00 100644 --- a/apps/esmtool/esmtool.cpp +++ b/apps/esmtool/esmtool.cpp @@ -52,7 +52,9 @@ int main(int argc, char**argv) cout << "Author: " << esm.getAuthor() << endl; cout << "Description: " << esm.getDesc() << endl; - cout << "File format version: " << esm.getFVer() << endl; + cout << "File format version: " << ((esm.getVer()==VER_12) ? "1.2" : + ((esm.getVer()==VER_13) ? "1.3" : + (void*)esm.getVer())) << endl; cout << "Special flag: " << esm.getSpecial() << endl; cout << "Masters:\n"; ESMReader::MasterList m = esm.getMasters(); diff --git a/components/esm/esm_reader.hpp b/components/esm/esm_reader.hpp index 64e0861..ef708e3 100644 --- a/components/esm/esm_reader.hpp +++ b/components/esm/esm_reader.hpp @@ -151,7 +151,6 @@ public: *************************************************************************/ int getVer() { return c.header.version; } - float getFVer() { return *((float*)&c.header.version); } int getSpecial() { return spf; } const std::string getAuthor() { return c.header.author.toString(); } const std::string getDesc() { return c.header.desc.toString(); }
Zini wrote: Yeah, this patch looks good. Maybe you should fork OpenMW on github and push your changes to the fork.
Pretty much, yes, because else the expression 1.2f==1.2f could yield false. I am pretty sure we would heard about it, if there ever had been a compiler/hardware combination in modern times, where this could occur. It would have been all over the news, because the company responsible would have had an angry mob of computer scientists and mathematicians at their door, wielding pitchforks and torches.Some rounding is still occuring, whether it's done at run-time or compile-time. Can you be sure it'll round as you need it to?
Chris wrote:I'm not very familiar with using Git forks. I also don't plan on maintaining a separate branch.. just making and supplying random patches for things that I find.Zini wrote:Yeah, this patch looks good. Maybe you should fork OpenMW on github and push your changes to the fork.
...which is basically what I said above, why equality checks are a big no-no.Pretty much, yes, because else the expression 1.2f==1.2f could yield false.
Anyone that needs accuracy and exactness in fractional values would not be using IEEE floating-point. It's ripe with rounding problems, among other things. On another board I go to it's almost a monthly occurance that someone complains "OMG, I add 0.1 together 10 times, and it's doesn't equal 1.0???!?! Is my compiler broken????"I am pretty sure we would heard about it, if there ever had been a compiler/hardware combination in modern times, where this could occur. It would have been all over the news, because the company responsible would have had an angry mob of computer scientists and mathematicians at their door, wielding pitchforks and torches.