<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jul 12, 2017 at 1:28 PM Mandeep Singh Grang via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">mgrang marked 2 inline comments as done.<br>
mgrang added inline comments.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Support/ReverseIteration.h:26<br>
+ return ReverseIterate<bool>::value &&<br>
+ std::is_pointer<T>::value;<br>
}<br>
----------------<br>
dblaikie wrote:<br>
> 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)<br>
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.<br>
<br>
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).<br>
<br>
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.<br>
<br>
Could you please suggest some ways I can work around these?<br></blockquote><div><br>Using only the existing PointerLikeTypeTrait, here's a type trait that might detect if something has a PointerLikeTypeTrait specialization:<br><font face="monospace"><br></font><div><font face="monospace">#include <utility></font></div><div><font face="monospace">template <typename T> struct traits {};</font></div><div><font face="monospace"><br></font></div><div><font face="monospace">template <typename T> struct traits<T *> {</font></div><div><font face="monospace"> enum { foo = 3 };</font></div><div><font face="monospace">};</font></div><div><font face="monospace"><br></font></div><div><font face="monospace">template <typename T> struct is_pointer {</font></div><div><font face="monospace"> static char test_pointer(const void *);</font></div><div><font face="monospace"> template <typename T1></font></div><div><font face="monospace"> static typename std::enable_if<traits<T1>::foo != 0, char (&)[2]>::type</font></div><div><font face="monospace"> test_pointer(T1 *);</font></div><div><font face="monospace"> static const bool value = sizeof(test_pointer(std::declval<T *>())) == 2;</font></div><div><font face="monospace">};</font></div><div><font face="monospace"><br></font></div><div><font face="monospace">static_assert(is_pointer<void *>::value, "");</font></div><font face="monospace">static_assert(!is_pointer<int>::value, ""); </font><br><br>(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))<br><br>Though maybe Richard Smith or others have better ideas about how to do this.<br><br>- Dave<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: unittests/ADT/ReverseIterationTest.cpp:30<br>
+ // Check forward iteration.<br>
+ ReverseIterate<bool>::value = false;<br>
+ for (const auto &Tuple : zip(Map, Keys))<br>
----------------<br>
dblaikie wrote:<br>
> 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.<br>
><br>
> 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.<br>
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.<br></blockquote><div><br></div><div>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.<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Repository:<br>
rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D35043" rel="noreferrer" target="_blank">https://reviews.llvm.org/D35043</a><br>
<br>
<br>
<br>
</blockquote></div></div>