Sign conversion warnings

[CaspianA1]

When using the -Wsign-conversion flag and compiling with Clang, a bunch of sign conversion warnings are spit out. See below. The problems originate in x86.h, from these two lines:

// This is from line 57
#define GLMM_NEGZEROf 0x80000000 /*  0x80000000 ---> -0.0f  */

// This is from line 67
#define glmm_float32x4_SIGNMASK_NEG _mm_castsi128_ps(_mm_set1_epi32(0x80000000)) /* _mm_set1_ps(-0.0f) */

• The conversion warnings happen because GLMM_NEGZEROf, which expands to an integer type, overflows.

• The warnings can be resolved by changing the definition of GLMM_NEGZEROf to -2147483648, and by using this new defintiion of GLMM_NEGZEROf in glmm_float32x4_SIGNMASK_NEG (which should have been using that macro in the first place).

• I unfortunately am quite busy day-to-day, could someone please pick this up and make a PR?

lib/cglm/simd/x86.h:59:23: note: expanded from macro 'GLMM_NEGZEROf'
#define GLMM_NEGZEROf 0x80000000
                      ^~~~~~~~~~
lib/cglm/simd/x86.h:62:41: note: expanded from macro 'GLMM__SIGNMASKf'
   _mm_castsi128_ps(_mm_set_epi32(X, Y, Z, W))
                    ~~~~~~~~~~~~~       ^
In file included from src/event.c:2:
In file included from include/data/constants.h:6:
In file included from lib/cglm/cglm.h:18:
In file included from lib/cglm/mat4.h:57:
lib/cglm/simd/sse2/mat4.h:307:8: warning: implicit conversion changes signedness: 'unsigned int' to 'int' [-Wsign-conversion]
  x8 = glmm_float32x4_SIGNMASK_NPNP;
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/cglm/simd/x86.h:66:54: note: expanded from macro 'glmm_float32x4_SIGNMASK_NPNP'
#define glmm_float32x4_SIGNMASK_NPNP GLMM__SIGNMASKf(GLMM_NEGZEROf, 0, GLMM_NEGZEROf, 0)
                                     ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/cglm/simd/x86.h:59:23: note: expanded from macro 'GLMM_NEGZEROf'
#define GLMM_NEGZEROf 0x80000000
                      ^~~~~~~~~~
lib/cglm/simd/x86.h:62:35: note: expanded from macro 'GLMM__SIGNMASKf'
   _mm_castsi128_ps(_mm_set_epi32(X, Y, Z, W))
                    ~~~~~~~~~~~~~ ^
src/event.c:2:
include/data/constants.h:6:
lib/cglm/cglm.h:21:
lib/cglm/affine.h:42:
lib/cglm/affine-mat.h:22:
lib/cglm/simd/sse2/affine.h:101:8: warning: implicit conversion changes signedness: 'unsigned int' to 'int' [-Wsign-conversion]
  x5 = glmm_float32x4_SIGNMASK_NEG;
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/cglm/simd/x86.h:69:69: note: expanded from macro 'glmm_float32x4_SIGNMASK_NEG'
#define glmm_float32x4_SIGNMASK_NEG _mm_castsi128_ps(_mm_set1_epi32(GLMM_NEGZEROf)) /* _mm_set1_ps(-0.0f) */
                                                     ~~~~~~~~~~~~~~ ^~~~~~~~~~~~~
lib/cglm/simd/x86.h:59:23: note: expanded from macro 'GLMM_NEGZEROf'
#define GLMM_NEGZEROf 0x80000000
                      ^~~~~~~~~~

[recp]

Hi @CaspianA1,

Good catch thanks, yes glmm_float32x4_SIGNMASK_NEG should also use GLMM_NEGZEROf

0x80000000 is easy to read, it could be written as (int)0x80000000 but -2147483648 is also fine, what about using INT_MIN ( if it is not platform dependent ) which is -2147483648?

// ...
#include <limits.h>
// ...

#define GLMM_NEGZEROf INT_MIN  /* 0x80000000 ( -2147483648 ) ---> -0.0f  */

[CaspianA1]

Hi @recp, I think that leaving GLMM_NEGZEROf as 0x80000000 would probably be fine; INT_MIN is platform-dependent, so we could make the definition of GLMM_NEGZEROf to this, and leave a comment above:

/* Note that `0x80000000` corresponds to `INT_MIN` for a 32-bit int. */
#define GLMM_NEGZEROf ((int) 0x80000000) /*  0x80000000 ---> -0.0f  */

Sounds good?

[recp]

Yes looks good, thanks for quick response, maybe a PR? :) otherwise I'll

[CaspianA1]

I'm quite busy the next week :( would you have time to do a PR?

[recp]

@CaspianA1 sure, the PR: https://github.com/recp/cglm/pull/311

[recp]

The PR is merged, feel free to bring any issues, ideas, feedbacks 🚀

[CaspianA1]

Awesome (: