Page MenuHomePhabricator

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

Authored by rengolin on Aug 16 2021, 5:17 AM.

Details

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.

Diff Detail

Event Timeline

rengolin created this revision.Aug 16 2021, 5:17 AM
rengolin requested review of this revision.Aug 16 2021, 5:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2021, 5:17 AM
rengolin retitled this revision from Fix type in DenseMap<BitVector, *> to math for V.size() to Fix type in DenseMap<BitVector, *> to match V.size().Aug 16 2021, 5:19 AM

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)

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...

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.

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).

rengolin updated this revision to Diff 366620.Aug 16 2021, 7:13 AM
rengolin retitled this revision from Fix type in DenseMap<BitVector, *> to match V.size() to Fix type in DenseMap<SmallBitVector, *> to match V.size().
aganea accepted this revision.Aug 16 2021, 7:22 AM

LGTM.

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

This revision is now accepted and ready to land.Aug 16 2021, 7:22 AM

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.

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

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 closed this revision.Aug 17 2021, 3:23 AM

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

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

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.

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!