<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Aug 9, 2016 at 11:41 AM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Aug 9, 2016 at 11:36 AM Tim Shen <<a href="mailto:timshen@google.com" target="_blank">timshen@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Aug 9, 2016 at 10:35 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<span style="line-height:1.5"> </span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>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:<br><br>TEST(ScopeExitTest, Basic) {</div><div>  bool Called = false;<br>  {<br>    auto g1 = make_scope_exit([&] { Called = true; });<br>    EXPECT_FALSE(Called);<br>  }<br>  EXPECT_TRUE(Called);<br>}<br><br></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>would suffice.<br></div></div></div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>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).</div><div><br></div><div>Imagine such user code:</div><div>{</div><div>  auto g1 = make_scope_exit([&] { Called1 = true; });</div><div>  auto g2 = make_scope_exit([&] { Called2 = true; });</div><div>}</div><div><br></div><div>If a wrongly implemented make_scope_exit executes g1 before g2, your test case won't detect that.</div></div></div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>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?<br><br>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.</div></div></div></blockquote><div><br></div><div>Fair enough.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br>(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)</div></div></div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>Sorry, it's testing moving the callable object into the scope_exit constructor, not the movability of the returned scope_exit object.</div></div></div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>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)<br></div></div></div></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br>- Dave</div></div></div><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: unittests/ADT/ScopeExitTest.cpp:37<br>
@@ +36,3 @@<br>
+  std::function<void()> f([&succ] { succ = true; });<br>
+  ASSERT_TRUE(static_cast<bool>(f));<br>
+  {<br>
----------------<br>
dblaikie wrote:<br>
> I take it this static cast is needed for some reason? (but isn't needed for EXPECT_FALSE?)<br>
Yes, otherwise it doesn't compile - it's something to do with ASSERT.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D22796" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22796</a><br>
<br>
<br>
<br>
</blockquote></div></div></blockquote></div></div></blockquote></div></div></blockquote></div></div>