Suspicious shift

[MarkCallow]

In basisu.h at line 310 is

        inline operator uint32_t() const
        {
            switch (NumBytes)
            {
                                 ...
                case 8:  
                {
                    uint32_t l = read_le_dword(m_bytes);
                    uint32_t h = read_le_dword(m_bytes + 4);
                    return static_cast<uint64_t>(l) | (static_cast<uint64_t>(h) << 32U);  <=========
                }
                                ...
            }
        }

When compiling for iOS arm64, the latest Xcode/clang (13.5 / 13.1.6) throws an implicit conversion loses integer precision warning which drew my attention to the highlighted line. This line looks extremely suspicious because the shift of h by 32-bits is throwing away the value of h in so far as the 32-bit return value will never see it. Many other cases also return a value that is statically cast to 64-bits which seems pointless as the return value is uint32_t. There is no warning on those cases.

If the highlighted line is truly correct, please add additional casts to fix the warnings, otherwise fix the code.

[richgel999]

Yes, that is strange looking. I'll investigate. I doubt it's causing any issues currently but could in the future.

[MarkCallow]

Why is this invalid? This will end up throwing away the high 32-bits. So why not just return l? If the shifting, OR'ing and casting is doing something useful that I'm missing at least wrap the whole thing in a static_cast to uint32_t to avoid compiler warnings.

I only get a compile warning when building for an iOS build-only device, which we only do in CI. Since I need to have warnings as errors turned on, this is enough to break our CI builds. I don't understand why that is the only case I see a warning.

P.S. I didn't get any notification of the label change from GitHub.

[MarkCallow]

Xcode 14.2 (clang 14.0.29) now throws the same warning for macOS builds. This issue is not invalid.

[richgel999]

Mark, It's impossible for us to support warnings as errors with all relevant compilers & compiler versions. If we fix a warning on compiler X, the fix could introduce a new warning on compiler Y. Then if we fix that warning, we could then introduce a new warning on compiler X. The warnings are not 100% standardized, so we cannot eliminate them all. On the next major build I'll do another sweep to cut down on the warnings with gcc/clang.

[MarkCallow]

In this case I'm actually asking you to fix the code not fix the warning. After finding the language spec regarding 64- to 32-bit casts impenetrable I ran some experiments to find out what compilers do. The ones I tested drop the high 32 bits, copying the low 32 to the destination. Therefore the code in each of the > 4 cases can be replaced with

return read_le_dword(m_bytes);

No need to extract the high bit or have the confusing shift, or and casts. And no warnings because nothing strange is happening.

What you say about warnings may be true in some cases. In this case though fixing the warning can be accomplished by wrapping the return expression in another cast

return static_cast<uint32_t>(static_cast<uint64_t>(l) | (static_cast<uint64_t>(h) << 32U));

which will work with any compiler.