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

[David Blaikie via llvm-commits]

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.


>
>
>>
>> (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)

- 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/84fa482b/attachment.html>