[PATCH] D112052: [ADT] Add Compare template param to EquivalenceClasses

[Matthias Springer via Phabricator via llvm-commits]

springerm marked an inline comment as done.
springerm added inline comments.


================
Comment at: llvm/include/llvm/ADT/EquivalenceClasses.h:106
+    bool operator<(const ECValue &UFN) const {
+      return Compare()(Data, UFN.Data);
+    }
----------------
ftynse wrote:
> It feels expensive to construct the comparator object on each call to `operator<`. Caching it in the `ECValue` isn't really a good option because it will consume a lot of space, even if we only keep a reference to the comparator stored in the `EquivalenceClassses`. Can't we just pass the template parameter down to `std::set`, potentially wrapped in some extra class? AFAIK, the set implementation will only keep one copy of the comparator.
Done.

I don't expect comparator classes to have state. So in most cases, I would not expect any overhead from instantiation. But you're right if the comparator happens to have state...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112052