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

[Tim Shen via llvm-commits]

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

> On Tue, Aug 9, 2016 at 11:36 AM Tim Shen <timshen at google.com> wrote:
>
>> 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.
>>
>
> That would have to be a pretty extreme wrong implementation though, right
> - it'd have to stash things in some global object and then execute them,
> etc?
>
> Otherwise it's only testing the language's guarantee that destructors run
> in reverse order. You don't need to test the compiler's correctness here -
> that's up to compiler tests.
>

Fair enough.


>
>
>>
>>
>>>
>>> (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.
>>
>
> Ah, OK. If that's worth testing you might want to implement a move-only
> type and/or a type that tracks which move/copy/etc operations execute and
> then test those counters. (we have a few such types in the ADT unit tests -
> maybe we should pull out one or two of them into common test utilities)
>

I looked at those MoveOnly structs - they are different enough to be kept
separate. In my case I need a callable object, so for this time I won't
extract MoveOnly out.


>
> - Dave
>
>
>>
>>
>>>
>>>
>>>>
>>>> ================
>>>> 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/a36d4701/attachment.html>