Fails to build with GCC 4.4.3

Support for running, installing or compiling OpenMW
Locked
User avatar
lgromanowski
Site Admin
Posts: 1193
Joined: 05 Aug 2011, 22:21
Location: Wroclaw, Poland
Contact:

Fails to build with GCC 4.4.3

Post by lgromanowski » 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:

Code: Select all

union {
    int someint;
    float somefloat;
};
someint = intvalue;
// somefloat now has the same byte values of someint
Just need to make sure the two types are the same size.
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

Code: Select all

int size;
it should be

Code: Select all

int size = 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 full error message please.

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:
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

Code: Select all

int size;
it should be

Code: Select all

int size = 0;
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):

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;
Though you'd probably want to put that under a macro, so MSVC can specify the appropriate __declspec (whatever it may be).
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 full error message please.

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.
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.

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:

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
(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.
Chris wrote: Here's essentially what I did to get by the errors before Boost:

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;
I don't like the memcpy's, but what ya gonna do?
Zini wrote:
void __attribute__((noreturn)) fail(const std::string &msg)
Uhm ... no? Unless profiling shows very clearly, that we have a bottleneck here, that can be avoided by not writing

Code: Select all

int size = 0;
(very unlikely btw.) code clarity is much more important and this line makes the code substantially harder to read.


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:

Code: Select all

if(n == "RIDT")
{
  Data2 data2; // argument order swapped
  esm.getHT (data2, 16);
  assign contents of data2 to data
}
This makes the fix entirely local and no code, that uses Tool has to be changed.

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:
Zini wrote:
void __attribute__((noreturn)) fail(const std::string &msg)
Uhm ... no? Unless profiling shows very clearly, that we have a bottleneck here, that can be avoided by not writing

Code: Select all

int size = 0;
(very unlikely btw.) code clarity is much more important and this line makes the code substantially harder to read.
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).
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.
The patch above fixes it by using two memcpy's and a tmp var. :)
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.
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
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.
Chris wrote:
Zini wrote:I don't like the memcpy either. ;) I think my proposed solution is cleaner.
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.

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... ;)
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.
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.
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
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.
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.
Zini wrote:
Comparing two floats for equality is a huge no-no
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.
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?).
This I fully agree with.
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..)
It is exactly as bad as the WINAPI crap.
Standard C++ doesn't define it, but if it's a completely normal compiler feature,
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.
More than enough reason to not use it.
it should be taken advantage of if it's available (IMO).
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.
nicolay wrote:
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
Thanks for the report. Strange, I'm using 4.4.3 too and I don't get any or these warnings / errors.

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.
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.
Ok. But I personally don't see any reason to go through elaborate steps to change this, it's completely valid C/C++ code.
Chris wrote:
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.
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:

Code: Select all

bool CheckVer(float ver)
{
    if(ver == 1.2)
        return true;
    return false;
}

...

if(CheckVer(1.2))
    ...
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).

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.
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.
You can say the same thing about any standard construct the coder may not be familiar with.

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.
nicolay wrote:
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
Thanks for the report. Strange, I'm using 4.4.3 too and I don't get any or these warnings / errors.
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..
Ok. But I personally don't see any reason to go through elaborate steps to change this, it's completely valid C/C++ code.
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).
Zini wrote:
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).
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.
Chris wrote:
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.
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:
And it still doesn't solve the problem of changes in FPU rounding mode.
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.
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.
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?
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.
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:
Zini wrote:
And it still doesn't solve the problem of changes in FPU rounding mode.
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.
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?
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?
Yes and no. Its no more obscure than anything else regarding floating point numbers.
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?
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.
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.

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.
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?
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.
Chris wrote:
Zini wrote:Yeah, this patch looks good. Maybe you should fork OpenMW on github and push your changes to the fork.
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.
Pretty much, yes, because else the expression 1.2f==1.2f could yield false.
...which is basically what I said above, why equality checks are a big no-no. ;) You can't be sure that a given compiler will take 1.2f and produce a bit-perfect clone of 0x3f99999a. Besides, if you're going to only use number literals and not do any math on them, you're better off using enums which can be properly named.
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.
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????"
best regards,
Lukasz

Locked