[PATCH] D35043: [ADT] Enable reverse iteration for DenseMap
Grang, Mandeep Singh via llvm-commits
llvm-commits at lists.llvm.org
Sun Jul 23 17:24:10 PDT 2017
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>
More information about the llvm-commits
mailing list