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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
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.");
+
----------------
dblaikie wrote:
> 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?
Probably, instead of warning we can introduce method:

```
bool IsInGoodState () const {
  return CurSlabNum <= MaxNumberOfSlabs && !hasFullSlab;
}


```

the consumer could call this method somewhere and inform the user:

```
if (!HashTable.IsInGoodState()) {
    outs() << "Hash table is in suboptimal state";
}
```

?


================
Comment at: llvm/include/llvm/ADT/ConcurrentHashTable.h:315-316
+///
+/// It is recommended to use ConcurrentHashTableExtAlloc if sizeof(KeyDataTy) >
+/// sizeof(pointer).
+///
----------------
dblaikie wrote:
> 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.
They have different interface:


```
std::pair<KeyDataTy, bool> insert(KeyDataTy NewData)
```
and
```
std::pair<KeyDataTy *, bool> insert(AllocatorTy &Allocator, KeyTy Key)
```

Implementing this through the specialization looks to be more complex and it seems does not have benefits comparing to the variant with separately named types?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125979