[PATCH] D22796: [ADT] Add make_scope_exit().

[Tim Shen via llvm-commits]

On Tue, Aug 9, 2016 at 10:35 AM David Blaikie <dblaikie at gmail.com> wrote:

> I'm trying to understand why testing 3 of them gives greater coverage than
> testing one. They're rather orthogonal & I would expect something like this:
>
> TEST(ScopeExitTest, Basic) {
>   bool Called = false;
>   {
>     auto g1 = make_scope_exit([&] { Called = true; });
>     EXPECT_FALSE(Called);
>   }
>   EXPECT_TRUE(Called);
> }
>
> would suffice.
>

Your test case doesn't test the order of execution (should be the reversed
order of the declarations of guards that are in the same scope).

Imagine such user code:
{
  auto g1 = make_scope_exit([&] { Called1 = true; });
  auto g2 = make_scope_exit([&] { Called2 = true; });
}

If a wrongly implemented make_scope_exit executes g1 before g2, your test
case won't detect that.


>
> (also, none of these necessarily test the copy (or move - I assume this
> isn't actually copyable?) support of the type, due to NRVO possibly)
>

Sorry, it's testing moving the callable object into the scope_exit
constructor, not the movability of the returned scope_exit object.


>
>
>>
>> ================
>> Comment at: unittests/ADT/ScopeExitTest.cpp:37
>> @@ +36,3 @@
>> +  std::function<void()> f([&succ] { succ = true; });
>> +  ASSERT_TRUE(static_cast<bool>(f));
>> +  {
>> ----------------
>> dblaikie wrote:
>> > I take it this static cast is needed for some reason? (but isn't needed
>> for EXPECT_FALSE?)
>> Yes, otherwise it doesn't compile - it's something to do with ASSERT.
>>
>>
>> https://reviews.llvm.org/D22796
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160809/55db1571/attachment.html>