[PATCH] D92584: [CSSPGO][llvm-profgen] Refactor to unify hashable interface for trace sample and context-sensitive counter

Thu Dec 10 15:14:36 PST 2020

hoy added inline comments.

================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:104
+  // Hash compare
+  struct Equal {
+    bool operator()(const Hashable<T> &LHS, const Hashable<T> &RHS) const {
----------------
wlei wrote:
> hoy wrote:
> > Nit: could this be defined as an overloaded `==` operator?
> `==` should work. Here using `Equal` is intentional, I want to diff from `==` to indicate that this is a 'Equal' function for hash not for the regular compare. Currently we exactly compare the elements like `==`, but maybe in the future if we have a good hash function, we can use the hash code for the comparison. Or we can have different custom `Equal` function, like `ContextKeyEqual`, `SampleEqual`. Or maybe the 'Equal' is not a proper naming.  Any thoughts on this?
> 
Sounds good. It makes sense to name the comparison more specifically.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92584