[PATCH] D156005: Use `getHashValue` in `SetVector::insert` and `SetVector::contains`

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

SetVector is templated over the set type, so using DenseMapInfo is not great.

In D156005#4524638 <https://reviews.llvm.org/D156005#4524638>, @etcwilde wrote:

> Sorry, should have used the the in the `DenseMapInfo<T>::isEqual`.
>
> So for background, the issue that I'm running into right now is two-fold, this container used to accept types that didn't necessarily have equality (`operator==`) (except as defined by the DenseMapInfo), which made it very useful for storing unique non-equatable things like non-canonical types in a type system. This change means that that code doesn't compile anymore. Second, the `DenseMapInfo<T>::isEqual` and `DenseMapInfo<T>::getHashValue` from the `DenseMapInfo` used before aren't necessarily the same as `T::operator==`, which can result in different outcomes.

My 2c here is that if you are implementing DenseMapInfo for a type, it needs to match `operator==` behavior. The only differences it should have from `operator==` is potentially different handling for empty / tombstone keys.

If you want to use a DenseMap with a different comparison criterion (or over a type that is not inherently comparable), then you should not implement DenseMapInfo for that type, but instead provide a separate DenseMapInfo implementation.

For example, see https://github.com/llvm/llvm-project/blob/c51c607dfc6388050ed1381a32089cd08a36ff9a/clang/include/clang/Sema/Weak.h#L37, which defines DenseMapInfoByAliasOnly for a type that is intentionally not comparable. You can see how it gets use (incidentally inside a SetVector) here: https://github.com/llvm/llvm-project/blob/c51c607dfc6388050ed1381a32089cd08a36ff9a/clang/include/clang/Sema/Sema.h#L1128 This sounds like it would be the proper thing to do in your case.

However, you can also just disable the optimization by passing N = 0 to SetVector (which is the default if you're not going through SmallSetVector). Using `SetVector<T, SmallVector<T, N>, SmallDenseSet<T, N>>` should give you exactly the same behavior as previously.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156005/new/

https://reviews.llvm.org/D156005