<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Sun, Jul 23, 2017 at 5:24 PM Grang, Mandeep Singh <<a href="mailto:mgrang@codeaurora.org">mgrang@codeaurora.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<p>Thanks David and others for all your comments and suggestions so
far.<br>
</p></div><div text="#000000" bgcolor="#FFFFFF">
<p>> 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.</p>
</div><div text="#000000" bgcolor="#FFFFFF"><p>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:<br>
</p>
<p>1. In the unit tests, how do we then check for the correct order
of iteration?</p></div></blockquote><div>I'd suggest hardcoding a specific order (based on observation) - this test necessarily is testing implementation details anyway - it does make it a little brittle, but some good comments about "update this list if the order changes". </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><p> 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.</p>
<p>2. If we make the variable a constant, we also need to get rid of
the -mllvm -reverse-iteration flag. </p></div></blockquote><div>Yep - I'm totally open to other opinions on this, maybe since it's already a runtime flag it can stay that way.<br><br>- Dave </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><p>This is not much of an issue
except that we won't have a quick way to check things.</p></div><div text="#000000" bgcolor="#FFFFFF">
<p>--Mandeep<br>
</p></div><div text="#000000" bgcolor="#FFFFFF">
<br>
<div class="m_572249993700642826moz-cite-prefix">On 7/15/2017 5:44 PM, David Blaikie
wrote:<br>
</div>
<blockquote type="cite">
<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" target="_blank">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>
</blockquote>
<br>
</div></blockquote></div></div>