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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/Support/ReverseIteration.h:26
+  return ReverseIterate<bool>::value &&
+         std::is_pointer<T>::value;
 }
----------------
Should this use an LLVM trait, to catch more interesting things that are pointer-like but not is_pointer? (I guess unique/shared ptr, and maybe PointerIntPair, PointerUnion, etc? Not sure if there's already an LLVM trait that covers all of those)


================
Comment at: unittests/ADT/ReverseIterationTest.cpp:23-24
+  DenseMap<void*, int> Map;
+  void *Keys[] = { (void*)0x1, (void*)0x2, (void*)0x3, (void*)0x4 };
+  void *ReverseKeys[] = { (void*)0x4, (void*)0x3, (void*)0x2, (void*)0x1 };
+
----------------
This, I assume, depends on the particular hash/iteration order of the container? Might be better to, like in the non-pointer test below, gather the order from the container & demonstrate it's the reverse, not that it's any particular order more specifically than that.

(also, I'm not quite sure on the rules for aliasing, etc, of creating pointers to arbitrary values like that - maybe declare 4 ints and have an array of int pointers to those ints?)

Also you can walk an array in reverse with llvm::reverse(TheArray)


================
Comment at: unittests/ADT/ReverseIterationTest.cpp:30
+  // Check forward iteration.
+  ReverseIterate<bool>::value = false;
+  for (const auto &Tuple : zip(Map, Keys))
----------------
Having this as a mutable global variable seems surprising to me - though maybe it is the right answer here for testing & the like, I'm not sure.

I'd be OK if it was a constant and this test tested the constant (yeah, that'd mean there would be no coverage for reverse iteration in a build that hadn't enabled reverse iteration, but that might be OK (I think of reverse iteration-clean like being valgrind/sanitizer clean - yeah, we might check in tests that are near-meaningless when run without sanitizers, but they test that this scenario is sanitizer clean when the sanitizers are enabled)) to decide which order to expect results in. Also maybe some comments explaining that this is testing implementation details, clients shouldn't depend on iteration order, and that these tests are here to ensure that the intentional unreliability of the iteration order is functioning.


================
Comment at: unittests/ADT/ReverseIterationTest.cpp:91
+  for (auto begin = Map.begin(), end = Map.end();
+             begin != end; i++) {
+    ASSERT_EQ(begin->first, IterKeys[i]);
----------------
Probably put '++i, begin++' as the loop increment (so you can have both i and begin in the loop header rather than splitting them (or could use 'i++, begin++' to make them look more consistent - not sure which is better here, since the goal is to do the 'interesting' thing with begin)

Also, calling it 'begin' might be confusing, since it starts at begin, but then moves - maybe 'mi' (for 'map iterator', but not sure)


Repository:
  rL LLVM

https://reviews.llvm.org/D35043