MSVC warning 4146 on radix sort about bvh HOT 13 CLOSED

Which warning? And where? Without more information I cannot solve this.

from bvh.

Sorry that's C4146 on the error.
in radix_sort.hpp

from bvh.

Which line?

from bvh.

Also I don't have MSVC, nor do I use it (I only have a CI pipeline to test on it, but it's run by GitHub CI, not locally), so it would help if you could give the actual content of the error message.

from bvh.

C4146 is the actual content of the warning. 'unary minus operator applied to unsigned type, result still unsigned'.

It's line 93 looks like the -y triggers it.

from bvh.

This is perfectly fine. The C++ standard states:

A computation  involving  unsigned  operands  can  never overflow, because  a  result  that  cannot
be  represented  by  the  resulting  unsigned  integer  type  is reduced  modulo  the  number  that  is
one  greater  than  the  largest  value  that  can  be represented by the resulting type.

So it's MSVC being stupid in its warnings. Negating unsigned numbers is of course not so common, but it's perfectly defined and has a precise meaning. Here it's used in order to make turn a floating point value into a key that can be sorted using integer radix sort. The only assumption there is that floats are represented with IEEE-754, which is reasonable I think.

from bvh.

@madmann91, don't you want to add #pragma warning ( disable : 4146 ) at the top of the file? This warning is threated as error and compilation fails with MSVC.

from bvh.

I don't want to enable/disable warnings like this. MSVC has a lot of spurious warnings anyway and who knows which ones will trigger the next time I touch this code (and also which ones I'll have to disable once I update the code if they no longer apply). However, I also see that this can be annoying if you compile with warnings as errors (which I wouldn't recommend in general -- but you do you), so I can think of the following compromise: Try the patch below, if it works create a PR and I'll merge it. If not, well, try something in the same vein, basically negating a signed integer and putting that back into an unsigned one.

// replace this version...
static typename SizedIntegerType<sizeof(T) * CHAR_BIT>::Unsigned make_key(T x) {
    using U = typename SizedIntegerType<sizeof(T) * CHAR_BIT>::Unsigned;
    auto mask = U(1) << (sizeof(T) * CHAR_BIT - 1);
    auto y = as<U>(x);
    return (y & mask ? (-y) ^ mask : y) ^ mask;
}
// ...with that version
static typename SizedIntegerType<sizeof(T) * CHAR_BIT>::Unsigned make_key(T x) {
    using U = typename SizedIntegerType<sizeof(T) * CHAR_BIT>::Unsigned;
    using I = typename SizedIntegerType<sizeof(T) * CHAR_BIT>::Signed;
    auto mask = U(1) << (sizeof(T) * CHAR_BIT - 1);
    auto y = as<U>(x);
    return (y & mask ? static_cast<U>(-static_cast<I>(y)) ^ mask : y) ^ mask;
}

from bvh.

Looks like now it works same way but without warning. I checked it with numbers which are close to 2^n (sign bit is used). And also compared traverse statistic - it was the same.

from bvh.

How do you make PR here? I can't create branch, but i see merged PRs like that:

madmann91 merged 1 commit into madmann91:master from newr5:master

I have no idea how to do this trick.

from bvh.

You can click the fork button (top right when on the project page), then this will create your own fork of the project. Then you can commit into it via normal means. Once that is done, you can go on to your fork in the web browser and create a pull request from there. I'll review it and merge it. Once that is done then you can delete your fork. Thank you for putting that extra effort!

from bvh.

Done! Thank you for cooperation.

from bvh.

Should be fixed by commit 93a7cdc

from bvh.