[PATCH] D37128: [unittests] Limit reverse iteration test to only reverse iteration builds

[David Blaikie via llvm-commits]

Yep - this is essentially impossible to test for using raw pointers. The
test will be unreliable in either forward or reverse direction. Please
commit a change to delete the tests that test containers of pointers. (no
need to wait for code review, etc)

In terms of finding a more reliable/good way to test this functionality...
- perhaps it'd be possible to make a custom type that's pointer-like, but
not a pointer & has known comparisons. (eg: struct foo { int i; }; with an
IsPointerLike specialization, comparison/hashing using the member int so
it's reliably ordered)

On Thu, Aug 24, 2017 at 5:36 PM Grang, Mandeep Singh <mgrang at codeaurora.org>
wrote:

> Compare these two:
>
>
> http://lab.llvm.org:8011/builders/clang-ppc64le-linux/builds/9064/steps/ninja%20check%201/logs/FAIL%3A%20LLVM-Unit%3A%3AReverseIterationTest.DenseMapTest1
>
>
> http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/4861/steps/test-stage1-compiler/logs/stdio
>
> In one case it expects "3" while in the other it expects "4".
>
> On 8/24/2017 5:31 PM, Grang, Mandeep Singh via llvm-commits wrote:
>
> I think maintaining an *expected* order for pointer-like keys may not be
> the correct way of doing this. We would need a way to make the expected
> order independent of the hashing function.
>
> On 8/24/2017 5:26 PM, Grang, Mandeep Singh via llvm-commits wrote:
>
> The problem is not the reversal of the expected order. The problem is that
> the *expected* order for pointer-like keys may differ based on how they are
> hashed.
>
> The hashing order may be machine/platform/environment dependent.
>
> --Mandeep
>
> On 8/24/2017 5:17 PM, David Blaikie wrote:
>
> Doesn't the test only reverse the expected order if reverse iteration is
> enabled? So why is this failing on a forward iteration build?
>
> On Thu, Aug 24, 2017 at 5:13 PM Mandeep Singh Grang via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> mgrang created this revision.
>>
>> This test causes failures in forward iteration builds. This is because we
>> have hard-coded
>> the expected order of iteration of supported containers.
>>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D37128
>>
>> Files:
>>   unittests/Support/ReverseIterationTest.cpp
>>
>>
>> Index: unittests/Support/ReverseIterationTest.cpp
>> ===================================================================
>> --- unittests/Support/ReverseIterationTest.cpp
>> +++ unittests/Support/ReverseIterationTest.cpp
>> @@ -16,6 +16,8 @@
>>  #include "llvm/Support/ReverseIteration.h"
>>  #include "gtest/gtest.h"
>>
>> +#if LLVM_ENABLE_REVERSE_ITERATION
>> +
>>  using namespace llvm;
>>
>>  TEST(ReverseIterationTest, DenseMapTest1) {
>> @@ -109,3 +111,5 @@
>>    for (auto iter = Set.begin(), end = Set.end(); iter != end; iter++,
>> ++i)
>>      ASSERT_EQ(*iter, IterPtrs[i]);
>>  }
>> +
>> +#endif
>>
>>
>>
>
>
> _______________________________________________
> llvm-commits mailing listllvm-commits at lists.llvm.orghttp://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
>
> _______________________________________________
> llvm-commits mailing listllvm-commits at lists.llvm.orghttp://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170825/3f596dbb/attachment.html>