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

[Mandeep Singh Grang via Phabricator via llvm-commits]

mgrang marked 2 inline comments as done.
mgrang added inline comments.


================
Comment at: include/llvm/Support/ReverseIteration.h:26
+  return ReverseIterate<bool>::value &&
+         std::is_pointer<T>::value;
 }
----------------
dblaikie wrote:
> 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)
Yes, ideally this should use an LLVM trait. I tried adding a "bool IsPointerLikeType" field to each template specialization in PointerLikeTypeTraits.h. As a result I had to add this field to each definition/specialization of this template throughout llvm/clang/polly as its definitions are scattered throughout.

More pressingly, the compilation failed since it will now call PointerLikeTypeTrait<T>::IsPointerLikeType for a T which does not have a complete trait definition (like LLVMBasicBlockRef).

I also tried adding a similar field to each specialization in DenseMapInfo. But that would make it too specific as I would then need to do something like "DenseMapInfo<T>::IsPointerLikeType". This would make it difficult to generalize this for other containers.

Could you please suggest some ways I can work around these?


================
Comment at: unittests/ADT/ReverseIterationTest.cpp:30
+  // Check forward iteration.
+  ReverseIterate<bool>::value = false;
+  for (const auto &Tuple : zip(Map, Keys))
----------------
dblaikie wrote:
> 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.
Ideally, I want to make ReverseIteration a class and the "value" field a private member and introduce setter/getter functions. I am currently running through compilation failures with this. Will push a subsequent patch once I get a clean build.


Repository:
  rL LLVM

https://reviews.llvm.org/D35043