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

[David Blaikie via llvm-commits]

On Mon, Aug 8, 2016 at 6:53 PM Tim Shen <timshen at google.com> wrote:

> timshen added inline comments.
>
> ================
> Comment at: unittests/ADT/ScopeExitTest.cpp:21-31
> @@ +20,13 @@
> +TEST(ScopeExitTest, Copy) {
> +  std::vector<int> v;
> +  const auto f = [&](int counter) {
> +    return [&v, counter] { v.push_back(counter); };
> +  };
> +  {
> +    auto g1 = make_scope_exit(f(0));
> +    { auto g2 = make_scope_exit(f(1)); }
> +    auto g3 = make_scope_exit(f(2));
> +    EXPECT_EQ((std::vector<int>{1}), v);
> +  }
> +  EXPECT_EQ((std::vector<int>{1, 2, 0}), v);
> +}
> ----------------
> dblaikie wrote:
> > This test seems a bit complicated - lambdas producing lambdas, a
> std::vector, etc.
> >
> > What are the actual behaviors you're trying to test here?
> Sorry, if std::vector raises surprises, it's a mistake. Changed to
> SmallVector.
>

Oh, sorry, no - it's not the difference between std::vector and
SmallVector. It's the existence of a vector at all in a test for a scope
device like this. It seems like more work than I'd expect to need.


>
> I'd like to test the order of the scope_exit runs. I label each scope_exit
> with a number (x for make_scope_exit(f(x))), and when the scope exits, the
> label is pushed to the vector "v". Then I inspect v to make sure of the
> order of labels.


> Do you have any suggestions to make it simpler?
>

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.

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


>
> ================
> 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/07be42e9/attachment.html>