[libcxx-commits] [PATCH] D128021: [libc++] Don't call key_eq in unordered_map/set rehashing routine

[Mark de Wever via Phabricator via libcxx-commits]

Mordante requested changes to this revision.
Mordante added a comment.
This revision now requires changes to proceed.

I think this patch is getting close to be done!

I see some small issues.
Can you provide the new binary sizes, I would like to see the overhead of this new approach.
Can you upload the patch with the complete context? Without context it's hard to validate whether the changes to `unordered_set` and `unordered_map` are correct. (I assume they are, but I want to validate.)



================
Comment at: libcxx/benchmarks/unordered_set_operations.bench.cpp:110
+  inline TEST_ALWAYS_INLINE
+  bool operator()(const std::string& lhs, const std::string& rhs) const {
+      if (lhs.size() != rhs.size()) return false;
----------------
This function is confusion. It's called slow, but when the sizes don't match they function bails out quickly. Else it compares all elements. Some comment would be helpful.


================
Comment at: libcxx/benchmarks/unordered_set_operations.bench.cpp:114
+      bool eq = true;
+      for (size_t i = 0; i < lhs.size(); ++i) eq &= lhs[i] == rhs[i];
+      return eq;
----------------
please put `eq &= lhs[i] == rhs[i];` on its own line.


================
Comment at: libcxx/include/__hash_table:1155
     void clear() _NOEXCEPT;
-    void rehash(size_type __n);
-    _LIBCPP_INLINE_VISIBILITY void reserve(size_type __n)
-        {rehash(static_cast<size_type>(ceil(__n / max_load_factor())));}
+    _LIBCPP_INLINE_VISIBILITY void rehash_unique(size_type __n) { __rehash<true>(__n); }
+    _LIBCPP_INLINE_VISIBILITY void rehash_multi(size_type __n) { __rehash<false>(__n); }
----------------
please us `__ugly` function names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128021