Comment by nkurz | Hacker News Reader

Comment by nkurz | Hacker News Reader

Parent Full thread

No, that doesn't catch it because "float_t" is properly defined as a 'float' by including <math.h>, instead of a 'double' in the local header. But I thought you might be on to something with adding extra warnings, so I tried it out on http://gcc.godbolt.org.

Nope, no warnings at all for spectral_contrast.c with GCC, ICC, or Clang even with "-Wall -Wextra -pedantic". Then I thought to try it on MSVC at http://webcompiler.cloudapp.net/. To my surprise, it caught it pretty clearly with /W4:

  main.cpp(11): warning C4244: '/=': conversion from 'double' 
                to 'float_t', possible loss of data

With that hint, I searched for other GCC warnings and found -Wconversion. Indeed, with that (or -Wfloat-conversion) GCC picks up the scent pretty well: http://goo.gl/9xq3fG

   In function 'void normalize(float_t*, int)':
   11 : warning: conversion to 'float_t {aka float}' from 
        'double' may alter its value [-Wfloat-conversion]

ICC gives an excellent error message with -Wconversion also, perhaps the clearest of the bunch: http://goo.gl/cjXLjq

   warning #2259: non-pointer conversion from "double" to 
                  "float_t={float}" may lose significant bits
       for(i = 0; i < length; i++) v[i] /= magnitude;

Clang remained silent with all the options I tried, but perhaps I missed the right one.

I tried to compile it, and I also couldn't find any other flag, than -Wconversion, that would detect this:

  for(i = 0; i < length; i++) sum += a[i] * b[i];
                                     ^

I managed to hide the error if return values, which are double, are replaced with float_t. I believe in that case the bug stays in, but -Wconversion doesn't detect it.

Of course the warning can always be silenced by doing something like:

  for(i = 0; i < length; i++) sum += ( double )( a[i] * b[i] );

and adding a misleading comment about the explicit cast.

The interesting thing about changing return types is that you can alter the return type of spectral_contrast to float_t without affecting behavior, since float is promoted to double for argument passing (it's passed and returned in the xmm registers).

Did you try with -Weverything on Clang? That will enable more warnings than -Wall and -Wextra.

    $ clang -Wall -Weverything -pedantic -c -o spectral_contrast.o spectral_contrast.c
    spectral_contrast.c:16:8: warning: no previous prototype for function 'spectral_contrast' [-Wmissing-prototypes]
    double spectral_contrast(float_t *a, float_t *b, int length) {
           ^
    1 warning generated.

Thanks for mentioning that option. I hadn't known about it.

Clang does give a warning with -Weverything, but I don't understand what it means: http://goo.gl/M9qjmx

  13 : warning: no previous prototype for function spectral_contrast' [-Wmissing-prototypes]
       double spectral_contrast(float_t *a, float_t *b, int length) {

It gives the same warning if I change all the float_t's to float, or if I change all the float_t's to double, so I think it's not actually useful or relevant.

It's effectively catching the fact that `spectral_contrast.c` does not `#include "match.h"` — this is indeed directly a part of the underhandedness.

The warning occurs because the function is not static — and therefore callable by other modules. Since it's missing a forward definition (a previous prototype), those modules must blindly declare the prototypes themselves… and their prototypes can get out of sync with the actual definition. That's what's happening here. `match.h` has the forward prototype, but it's very subtly different from the definition. Were match.h included, there'd be a much more glaring warning (or maybe even error).

Reply on news.ycombinator.com