[PATCH] D125979: [ADT] add ConcurrentHashTable class.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/ConcurrentHashTable.h:83
+      std::function<void(const char *message)> WarningHandler = nullptr)
+      : WarningHandler(WarningHandler) {}
+
----------------
(similarly below/for the other ctor here)


================
Comment at: llvm/include/llvm/ADT/ConcurrentHashTable.h:117-118
+    else if (CurSlabNum > MaxNumberOfSlabs)
+      reportWarning("Too many slabs. Consider increasing initial size or "
+                    "changing hash function.");
+
----------------
It's pretty weird to have warnings coming out of data structures. @MaskRay would it be reasonable to remove these/replace them with some other tool if we're promoting this to a more general purpose data structure rather than something more special-case in lld?


================
Comment at: llvm/include/llvm/ADT/ConcurrentHashTable.h:315-316
+///
+/// It is recommended to use ConcurrentHashTableExtAlloc if sizeof(KeyDataTy) >
+/// sizeof(pointer).
+///
----------------
This should probably be provided via specialization, rather than as a separately named type?

I guess it has different requirements for the type, by the looks of it? That's curious/maybe enough to warrant a separate name, I guess, though I'm not 100% sure.


================
Comment at: llvm/include/llvm/ADT/ConcurrentHashTable.h:324
+/// hash_code hash_value(KeyTy Key);
+/// bool KeyTy::operator==(const KeyTy &Other) const;
+/// KeyTy KeyDataTy::key() const;
----------------
generally operator== should be implemented as a non-member (so that implicit conversions are equally applied to the left and right hand operands) - maybe this whole specification area should be written less in terms of specific declarations that should be provided, and more in terms of code snippets that should be valid/have certain semantics? (that's how the C++ standard documents requirements on things like value types, iterator types, etc)


================
Comment at: llvm/unittests/ADT/ConcurrentHashTableTest.cpp:44-46
+  bool operator==(const OrderedInt &Other) const {
+    return Data == Other.Data && Order == Other.Order;
+  }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125979