<br><br><div class="gmail_quote">On Fri, Dec 26, 2008 at 8:06 PM, Misha Brukman <span dir="ltr"><<a href="mailto:brukman@gmail.com">brukman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="Ih2E3d">On Dec 22, 7:34 pm, Talin <<a href="mailto:viri...@gmail.com">viri...@gmail.com</a>> wrote:<br>
> (Forwarding this to llvm-dev)<br>
><br>
> This patch adds a unit test framework to LLVM, along with a sample unit test<br>
> for DenseMap. I don't expect this patch to be accepted as-is, this is mainly<br>
> a trial balloon and proof of concept.<br>
<br>
</div>I think this is a great idea!  As Keir already noted, I would also<br>
agree with LLVM snapshotting a copy of googletest, but I think it<br>
should live in llvm/test/googletest (rather than top-level), since<br>
it's not going to be linked into anything outside of llvm/test.<br>
<br>
On the assumption that we'll end up using googletest, here are some<br>
comments on your patch:<br>
<br>
* s/Insure/Ensure/<br>
<br>
* LLVM uses "llvm/foo.h" for inclusion rather than <llvm/foo.h><br>
* You should use the same format for gtest headers<br>
<br>
* If reverse iteration isn't supported, you should either have an<br>
ASSERT_DEATH() on the decrement, or not have the code there (that's<br>
commented out) at all.<br>
<br>
* Instead of this:<br>
EXPECT_TRUE(uintMap[0] == 1);<br>
you should use EXPECT_EQ()<br>
<br>
* Instead of this:<br>
EXPECT_TRUE(uintMap.find(0u) == uintMap.begin());<br>
is it possible to use EXPECT_EQ() as well?<br>
<br>
* In this test:<br>
TEST_F(DenseMapTest, IterationTest) {<br>
you use the array "int numbers[100];" as an array of booleans; why not<br>
make it "bool visited[100];" to make clear what it for, how it's used,<br>
and maybe be slightly more efficient?<br>
<div class="Ih2E3d"><br>
> 1) For the testing framework, I went with Google Test, since it's the one I<br>
> have the most experience with. I fully expect an extended bikeshed<br>
> discussion to result from this.<br>
><br>
> 2) Both the test framework and the tests are optional build targets, they<br>
> will not be built with the normal "make all". To build and run the unit<br>
> tests, use "make check-unit".<br>
<br>
</div>I am with Keir on this one: "check-unit" isn't the most intuitive name<br>
to me -- "unittest" sounds fine, but this isn't a big deal to me.<br>
<br>
I'll also volunteer to help maintain the LLVM branch of googletest and<br>
review the unittests being checked in, as I also have experience with<br>
using googletest, and I think LLVM could derive a lot of value from<br>
unittesting.</blockquote><div><br>If we make useful changes to gtest, I volunteer to push them upstream.<br><br>Keir<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;">
<br>
<font color="#888888"><br>
Misha<br>
</font><div><div></div><div class="Wj3C7c">_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a>         <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
</div></div></blockquote></div><br>