Integral constant overflow in MSVC

[serge-sans-paille]

Integral constant overflow in MSVC

[uecasm]

It looks like changing the definition of elsa to this fixes it for 32-bit MSVC. I assume that it would be more complicated for 64-bit, since it would have to do a 128-bit add. Unfortunately there doesn't appear to be any way to suppress the warning inside elsa itself, if it's called in a constexpr context, since the warning is raised at the point of use, not at its own code.

#if defined(_MSC_VER) && !defined(_WIN64)
  constexpr std::size_t operator()(T const &value, std::size_t seed) const {
      std::size_t key = seed ^ static_cast<std::size_t>(value);
      key = static_cast<std::size_t>(static_cast<uint64_t>(~key) + (key << 21));
      key = key ^ (key >> 24);
      key = static_cast<std::size_t>(static_cast<uint64_t>(key) + (key << 3) + (key << 8));
      key = key ^ (key >> 14);
      key = static_cast<std::size_t>(static_cast<uint64_t>(key) + (key << 2) + (key << 4));
      key = key ^ (key >> 28);
      key = static_cast<std::size_t>(static_cast<uint64_t>(key) + (key << 31));
      return key;
  }
#else
  constexpr std::size_t operator()(T const &value, std::size_t seed) const {
    std::size_t key = seed ^ static_cast<std::size_t>(value);
    key = (~key) + (key << 21); // key = (key << 21) - key - 1;
    key = key ^ (key >> 24);
    key = (key + (key << 3)) + (key << 8); // key * 265
    key = key ^ (key >> 14);
    key = (key + (key << 2)) + (key << 4); // key * 21
    key = key ^ (key >> 28);
    key = key + (key << 31);
    return key;
  }
#endif

[serge-sans-paille]

I'd say overflow on a plus on unsigned type is not an issue because it's perfectly defined (unlike the signed version). What could be done instead of your cast is to apply a mask, as in (~key + (key << 21)) & ~std::size_t(0), does that silent the warning? or maybe static_caststd::size_t(~key + (key << 21)` ? If either of these work, feel free to open a PR!

[uecasm]

No, those don't help; but I wouldn't really expect them to, since the warning is raised for the + operator itself, not the assignment back into key.

[degski]

@serge-sans-paille clang-cl doesn't warn on this. Maybe just suppress the warning?

[uecasm]

Sadly the warning is raised at point of constexpr variable use, not anywhere inside elsa itself, as I mentioned above. This makes it impossible to suppress the warning inside the library, except by changing the operand types of + as proposed above so that overflow can't occur.

I did file a bug with MS about the warning behaviour, so we can just sit on it and see if anything comes of that.

[serge-sans-paille]

On Thu, Feb 21, 2019 at 10:28:46PM -0800, degski wrote: @serge-sans-paille clang-cl doesn't warn on this. Maybe just suppress the warning?

You mean, using a pragma, or using a compiler flag? Can we disable warning locally?

[degski]

@serge-sans-paille Yes, something like this, there are various options, more or less portable.

[uecasm]

@degski So, how many different ways do you want me to tell you that pragma suppressions don't work? Because this is the third time I've said that.