KhronosGroup / OpenCOLLADA

654 stars 252 forks source link

OpenCOLLADA fails to build with GCC 6 #439

Open DimStar77 opened 8 years ago

DimStar77 commented 8 years ago

GCC uses C++11 by default and this makes OpenCOLLADA fail the build with:

[ 70s] /home/abuild/rpmbuild/BUILD/OpenCOLLADA-3335ac164e68b2512a40914b14c74db260e6ff7d/COLLADABaseUtils/src/COLLADABUURI.cpp:57:2: error: narrowing conversion of '-1' from 'int' to 'char' inside { } [-Wnarrowing]

The code in question is:

    const char HEX2DEC[256] = 
    {
        /*       0  1  2  3   4  5  6  7   8  9  A  B   C  D  E  F */
        /* 0 */ -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1,

The 'issue' is that 'char' is not defined to be signed/unsigned (it depends on arch / implementation) (i586/x86_64 use signed char, ppc64le and arm for example uses unsigned char)

It would likely be safe to change this to const signed char HEX2DEC[256], this having all archs behave the same.

RemiArnaud commented 8 years ago

@mbarnes-sb for comment @jerome-jacob as we want to make openCOLLADA lib / validator C++ 11 compliant

mbarnes-sb commented 8 years ago

Unfortunately, signed char and unsigned char and char are incompatible types. Changing this type could snowball into more compiler errors (with iostream especially).

What is the reason for using -1 literals here? Use a different sentinel value instead? This is effectively trying to use std::char_traits<char>::eof() which is int_type not char_type.

hobbes1069 commented 8 years ago

Having implemented this change on Fedora in order to get a good build I have not run into any additional compiler errors. Can you be more specific on your concerns?

mbarnes-sb commented 8 years ago

Implicit conversion from signed char to either of the other types could cause compiler errors. if that's not happening then okay (wait and see I guess).

DimStar77 commented 8 years ago

that's why my proposal is to explicitly mark the type as being signed char (otherwise trying to fit -1 into it is as useless as anything).

without explicitly specifying signed char, we actually do not know if the architecture uses signed or unsigned char... which leaves you even more in the mud

For reference, openSUSE has been building opencollada with this patch for a while now: https://build.opensuse.org/package/view_file/openSUSE:Factory/openCOLLADA/openCOLLADA-signed-char.patch?expand=1

=> for i586/x86_64, there is no difference, as 'char' == 'signed char' for those anyway => for ppc64le it makes the build actually work, as 'char' == 'unsigned char' on this platform (with gcc) and unsigned char can't hold '-1'. Specifying 'signed char' instead makes it defined