Fix type in DenseMap<SmallBitVector, *> to match V.size()

[rengolin]

Summary

There are many cases in LLVM and Clang where implicit conversions from size_t to unsigned are assumed correct.

This handles one case where it isn't, by changing the template type to size_t to match V.size().

This was caught by MSVC 19.

I can send a fix for the other warnings later on a separate patch.

[serge-sans-paille]

SmallBitVector size method indeed return a size_t, but in a great show of consistency, BitVector doesn't (it returns size_type which aliases to unsigned)

[rengolin]

In D108124#2946664, @serge-sans-paille wrote:

SmallBitVector size method indeed return a size_t, but in a great show of consistency, BitVector doesn't (it returns size_type which aliases to unsigned)

Ugh. Worse, SmallBitVector.size() returns size_t getSmallSize() or size_type getPointer()->size()...

Ok, so we have two choices:

1. Change SmallBitVector to return size_type
2. Change BitVector to return size_t

I don't have enough information to pick, but given that the "smaller" version returns a potentially wider type, I think option 2 is best.

Either way, we also need to propagate the types across the users, so it won't be a simple patch as I wanted...

[rengolin]

Or, being conservatively (with container sizes), we can use size_type size() on both cases, because that's what the typedef seems to imply in the first place.

It's also the type of count() for both versions, which is equal or larger than size, so size_t is overkill.

[rengolin]

Nope, user code all over LLVM is expecting one to return size_t and the other unsigned, so I'll leave that major refactory to another day.

For now, I'll just change the one that is clearly wrong (SmallBitVector).

[aganea]

LGTM.

Would you mind posting the warning message in the patch description, for future reference? Thanks!

[rengolin]

This is the warning:

include\llvm/ADT/SmallBitVector.h(725): note: see reference to function template instantiation 'std::pair<unsigned int,llvm::ArrayRef<uint64_t>>::pair<unsigned __int64,llvm::ArrayRef<uint64_t>,0>(std::pair<unsigned __int64,llvm::ArrayRef<uint64_t>> &&) noexcept' being compiled
include\llvm/ADT/SmallBitVector.h(724): note: see reference to function template instantiation 'std::pair<unsigned int,llvm::ArrayRef<uint64_t>>::pair<unsigned __int64,llvm::ArrayRef<uint64_t>,0>(std::pair<unsigned __int64,llvm::ArrayRef<uint64_t>> &&) noexcept' being compiled
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30037\include\utility(249): warning C4244: 'initializing': conversion from '_Ty' to '_Ty1', possible loss of data
    with
    [
        _Ty=uint64_t
    ]
    and
    [
        _Ty1=unsigned int
    ]

Newer versions of MSCV find even more of those on both Clang (AST, Sema, Loc) and LLVM (Support, ADT). I've checked a few and they're all legitimate type mismatches.

But fixing them is as unwinding as this one, because there's a lot of (LLVM) user code that makes the same assumptions (ex. unsigned == size_t) and we'd need to chain all assumptions down to all users.

[serge-sans-paille]

Not the bravest version of the change, but obviously LGTM too ;-)

[rengolin]

In D108124#2948699, @serge-sans-paille wrote:

Not the bravest version of the change, but obviously LGTM too ;-)

:D

I'll do a better one soon, I just needed this fix to pass our CI.

[rengolin]

Not sure why my differential comment didn't pick up, but this was closed by a19747ea739

[teemperor]

In D108124#2948976, @rengolin wrote:

Not sure why my differential comment didn't pick up, but this was closed by a19747ea739

You created a new branch on the LLVM Github repo and I think that is confusing Phabricator: https://github.com/llvm/llvm-project/tree/size_t-warning

[rengolin]

In D108124#2949130, @teemperor wrote:

In D108124#2948976, @rengolin wrote:

Not sure why my differential comment didn't pick up, but this was closed by a19747ea739

You created a new branch on the LLVM Github repo and I think that is confusing Phabricator: https://github.com/llvm/llvm-project/tree/size_t-warning

ooops. Sorry, deleted the branch.

[teemperor]

In D108124#2949148, @rengolin wrote:

In D108124#2949130, @teemperor wrote:

In D108124#2948976, @rengolin wrote:

Not sure why my differential comment didn't pick up, but this was closed by a19747ea739

You created a new branch on the LLVM Github repo and I think that is confusing Phabricator: https://github.com/llvm/llvm-project/tree/size_t-warning

ooops. Sorry, deleted the branch.

No worries!

llvm/include/llvm/ADT/SmallBitVector.h

   }
   static unsigned getHashValue(const SmallBitVector &V) {
     uintptr_t Store;
-    return DenseMapInfo<std::pair<unsigned, ArrayRef<uintptr_t>>>::getHashValue(
+    return DenseMapInfo<std::pair<size_t, ArrayRef<uintptr_t>>>::getHashValue(
         std::make_pair(V.size(), V.getData(Store)));
   }
   static bool isEqual(const SmallBitVector &LHS, const SmallBitVector &RHS) {