[PATCH] D35043: [ADT] Enable reverse iteration for DenseMap

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Jul 30 00:23:22 PDT 2017


dblaikie added inline comments.


================
Comment at: include/llvm/ADT/DenseMap.h:73
+    if (ReverseIteration::shouldRevIter<KeyT>())
+      return makeIterator(getBucketsEnd() - 1, getBuckets(), *this);
+#endif
----------------
Should this have the "is empty" special case? Could the is empty case be moved above this #if ?

  if (empty())
    return end();
  #if LLVM_ENABLE_ABI_BREAKING_CHECKS
  ...
  #endif
  return makeIterator(getBuckets(), getBucketsEnd(), *this);

Similarly for the other ops below



================
Comment at: include/llvm/ADT/DenseMap.h:1134-1138
+      if (!NoAdvance) RetreatPastEmptyBuckets();
+      return;
+    }
+#endif
     if (!NoAdvance) AdvancePastEmptyBuckets();
----------------
Might be able to factor out the NoAdvance into an early return:

    if (NoAdvance) return;
  #if BREAKING_CHECKS
    if (reverse) {
      retreat();
      return;
    }
  #endif
    advance()


================
Comment at: include/llvm/Support/PointerLikeTypeTraits.h:37-53
+
+// Provide a trait for checking whether T is pointer-like.
+template <typename T> struct traits {};
+template <typename T> struct traits<T *> {
+  enum { x = 1 };
+};
+
----------------
Was it not possible to use something more like what I'd suggested - that would've worked (I think) with PointerLikeTypeTraits? Or did that not seem to work out?

Sounded like Richard had an idea of how to do it even more simply than my suggestion, maybe... (if we removed the base definition of PointerLikeTypeTraits & were able to test on the completeness of the class, rather than checking for the specific member as I'd done)


================
Comment at: include/llvm/Support/ReverseIteration.h:9-22
+class ReverseIteration {
+private:
+  ReverseIteration() {}
+  static bool RevIter;
+
+public:
+  template<class T = void*>
----------------
This is still a mutable dynamic property - is that necessary/good? Or could it be a constant (indeed having a standalone function template "shouldReverseIterate<T>" that has the value baked in by the macro in the header - no need for an out of line definition, etc)


================
Comment at: include/llvm/Support/ReverseIteration.h:11
+private:
+  ReverseIteration() {}
+  static bool RevIter;
----------------
= default


================
Comment at: lib/Support/ReverseIteration.cpp:8-12
+#if LLVM_ENABLE_REVERSE_ITERATION
+bool ReverseIteration::RevIter = true;
+#else
+bool ReverseIteration::RevIter = false;
+#endif
----------------
I still wonder if it'd be better if this was not mutable - a compile time constant, this would also potentially remove the need for all the preprocessor work. It could rely on the reverse iteration being a constant (not a command line arg, or mutable internally) and rely on the compiler doing dead code elimination to remove the unnecessary code instead of #if.

This would limit the ability to test the feature - really the feature would only be tested on buildbots/configurations that had reverse iteration enabled, or not, etc. But I think that's probably OK?


Repository:
  rL LLVM

https://reviews.llvm.org/D35043





More information about the llvm-commits mailing list