Hi Nuno,<br><br>Thanks for adding unittests! I have some comments below.<br><br><div class="gmail_quote">2009/1/2 Nuno Lopes <span dir="ltr"><<a href="mailto:nunoplopes@sapo.pt">nunoplopes@sapo.pt</a>></span><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Author: nlopes<br>
Date: Fri Jan 2 07:49:50 2009<br>
New Revision: 61576<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=61576&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=61576&view=rev</a><br>
Log:<br>
fist short at a new unit test for ImmutableSets. no bugs found, though :P<br>
<br>
Added:<br>
llvm/trunk/unittests/ADT/ImmutableSetTest.cpp<br>
<br>
Added: llvm/trunk/unittests/ADT/ImmutableSetTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/ImmutableSetTest.cpp?rev=61576&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/ImmutableSetTest.cpp?rev=61576&view=auto</a><br>
<br>
==============================================================================<br>
--- llvm/trunk/unittests/ADT/ImmutableSetTest.cpp (added)<br>
+++ llvm/trunk/unittests/ADT/ImmutableSetTest.cpp Fri Jan 2 07:49:50 2009<br>
@@ -0,0 +1,208 @@<br>
+// llvm/unittest/ADT/ImmutableSetTest.cpp - ImmutableSet unit tests -*- C++ -*-//<br>
</blockquote><div><br>You can put the description below the typical file header to make this line look standard, but this is minor.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+namespace {<br>
+class ImmutableSetTest : public testing::Test {<br>
+};<br>
+<br>
+<br>
+TEST_F(ImmutableSetTest, EmptyIntSetTest) {<br>
+ ImmutableSet<int>::Factory f;</blockquote><div><br>I see you use a Factory in every test -- you can make this a member of the ImmutableSetTest above, declare it once, and drop the declaration from every specific test -- that will make your code shorter. A new Factory will be created for each test, so you don't need to worry about getting a used Factory. :-)<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">+static char *ptr; // tmp var<br>
+struct MyIter {<br>
+ int counter;<br>
+ MyIter() : counter(0) {}<br>
+ void operator()(char c) {<br>
+ *ptr++ = c;<br>
+ ++counter;<br>
+ }<br>
+};<br>
</blockquote><div><br>Global state is frowned upon -- if you don't properly clear it between tests, strange bugs will occur, and they will depend on the order of the test execution (which aren't guaranteed to execute in any particular order). This will lead to strange Heisenbugs.<br>
<br>Instead, you can make another Fixture class, and make this state a part of that class. If you set up that state, you'll know you're always getting a clean beginning state, and you'll have nothing to worry about.<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">+TEST_F(ImmutableSetTest, CallbackCharSetTest) {<br>
+ ImmutableSet<char>::Factory f;<br>
+ ImmutableSet<char> S = f.GetEmptySet();<br>
+<br>
+ ImmutableSet<char> S2 = f.Add(f.Add(f.Add(S, 'a'), 'e'), 'i');<br>
+ ImmutableSet<char> S3 = f.Add(f.Add(S2, 'o'), 'u');<br>
+<br>
+ char buffer[6] = {0};<br>
+ ptr = buffer;<br>
+ S3.foreach<MyIter>();<br>
+<br>
+ ASSERT_EQ(buffer[0], 'a');<br>
+ ASSERT_EQ(buffer[1], 'e');<br>
+ ASSERT_EQ(buffer[2], 'i');<br>
+ ASSERT_EQ(buffer[3], 'o');<br>
+ ASSERT_EQ(buffer[4], 'u');<br>
+ ASSERT_EQ(buffer[5], 0);<br>
</blockquote><div><br>No need to use ASSERT here, EXPECT will work just as well, and will tell you if you have multiple errors (e.g., you'll see if 2 of them were swapped, for instance, which ASSERT won't tell you.<br>
<br>Also, and this is very important, the expected value in ASSERT and EXPECT values should go FIRST (you do this correctly above, but not here). This is important because if this assertion fails, the error will print as:<br>
ASSERT failed:<br>expected value: <foo><br>actual value: <bar><br><br>If you mix and match the order of the values, you'll get confusing error messages.<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+ ASSERT_EQ(*I, i++);<br>
+ }<br>
+ ASSERT_EQ(i, 0);<br>
+<br>
+ i = 0;<br>
+ for (ImmutableSet<long>::iterator I = S2.begin(), E = S2.end(); I != E; ++I) {<br>
+ ASSERT_EQ(*I, i++);<br>
+ }<br>
+ ASSERT_EQ(i, 3);<br>
+<br>
+ i = 0;<br>
+ for (ImmutableSet<long>::iterator I = S3.begin(), E = S3.end(); I != E; I++) {<br>
+ ASSERT_EQ(*I, i++);<br>
+ }<br>
+ ASSERT_EQ(i, 6);<br></blockquote><div><br>Same here on the order of expected vs. actual values -- please reverse them.<br><br><br>Misha<br></div></div>