[PATCH] RFC: fail-fast iterators for DenseMap

Sanjoy Das sanjoy at playingwithpointers.com
Mon Mar 2 10:00:21 PST 2015


Thanks for the review!  I'll land this sometime in the next couple of days.


================
Comment at: include/llvm/ADT/DenseMap.h:1014
@@ -1005,3 +1013,3 @@
 
-  DenseMapIterator(pointer Pos, pointer E, bool NoAdvance = false)
-    : Ptr(Pos), End(E) {
+  DenseMapIterator(pointer Pos, pointer E, const DebugEpochBase *Epoch,
+                   bool NoAdvance = false)
----------------
chandlerc wrote:
> Is Epoch ever false here? If not, pass by reference?
Will do.

================
Comment at: include/llvm/ADT/EpochTracker.h:25-35
@@ +24,13 @@
+
+class DebugEpochBase {
+public:
+  void bumpEpoch() {}
+
+  class HandleBase {
+  public:
+    HandleBase() {}
+    explicit HandleBase(const DebugEpochBase *) {}
+    bool isHandleInSync() { return true; }
+  };
+};
+
----------------
chandlerc wrote:
> Looking at what this will do with doxygen and other things, I think it would be better to sink the #ifdef stuff inside the class and method bodies. That will ensure code navigation and such always have a consistent target. A very minor point though.
Frankly, I'd rather have an easy to understand .h file over easy to read doxygen, so I went against too many `#ifdef`s.  I'll push the `#ifdef`s inside class bodies if people actually complain about bad doxygen. :)

================
Comment at: include/llvm/ADT/EpochTracker.h:56
@@ +55,3 @@
+  /// instance.
+  void bumpEpoch() { ++Epoch; }
+
----------------
chandlerc wrote:
> What do you think about calling this "next"? or "increment"? I don't feel strongly, but "bump" seems fairly imprecise...
Will change to `incrementEpoch`

http://reviews.llvm.org/D7931

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list