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

[Grang, Mandeep Singh via llvm-commits]

Thanks David and others for all your comments and suggestions so far.

 > I'm still not sure it's the best idea for this to be mutable - what 
would it be like if it were a compile time constant & no ifdefs were 
required for the use - and only the reverse build configuration would 
test that the reverse mode worked, etc.

I am totally fine with making the reverse iteration variable a compile 
time constant which would be ON for reverse iteration builds and OFF by 
default. However, I have a couple of questions:

1. In the unit tests, how do we then check for the correct order of 
iteration? Let's assume we have a reverse iteration build. We insert 
elements {&a, &b, &c, &d} into a container and when we iterate the 
container the elements are {&c, &d &a, &b}. Now how do we check whether 
this is the correct order (reverse order) or not. Whereas currently, in 
the unit test we manually turn reverse iteration ON/OFF so we know the 
default order and the reverse order.

2. If we make the variable a constant, we also need to get rid of the 
-mllvm -reverse-iteration flag. This is not much of an issue except that 
we won't have a quick way to check things.

--Mandeep


On 7/15/2017 5:44 PM, David Blaikie wrote:
>
>
> On Wed, Jul 12, 2017 at 1:28 PM Mandeep Singh Grang via Phabricator 
> <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
>
>     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?
>
>
> Using only the existing PointerLikeTypeTrait, here's a type trait that 
> might detect if something has a PointerLikeTypeTrait specialization:
>
> #include <utility>
> template <typename T> struct traits {};
>
> template <typename T> struct traits<T *> {
>   enum { foo = 3 };
> };
>
> template <typename T> struct is_pointer {
>   static char test_pointer(const void *);
>   template <typename T1>
>   static typename std::enable_if<traits<T1>::foo != 0, char (&)[2]>::type
>   test_pointer(T1 *);
>   static const bool value = sizeof(test_pointer(std::declval<T *>())) 
> == 2;
> };
>
> static_assert(is_pointer<void *>::value, "");
> static_assert(!is_pointer<int>::value, "");
>
> (where 'foo' is 'NumLowBitsAvailable' (or you could make it work for 
> teh other functions in PointerLikeTypeTraits - or even to check that 
> all 3 are available before accepting that it's pointer like))
>
> Though maybe Richard Smith or others have better ideas about how to do 
> this.
>
> - Dave
>
>
>
>     ================
>     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.
>
>
> I'm still not sure it's the best idea for this to be mutable - what 
> would it be like if it were a compile time constant & no ifdefs were 
> required for the use - and only the reverse build configuration would 
> test that the reverse mode worked, etc.
>
> - Dave
>
>
>
>     Repository:
>       rL LLVM
>
>     https://reviews.llvm.org/D35043
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170723/9587ff00/attachment.html>