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

[David Blaikie via llvm-commits]

On Wed, Jul 12, 2017 at 1:28 PM Mandeep Singh Grang via Phabricator <
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/20170716/63f0e3fc/attachment.html>