On Dec 30, 2008, at 2:54 PM, Talin wrote:> OK changes made and new patch attached.Nice +++ utils/unittest/Makefile (revision 0) ... +# This has to come after Makefile.common, since it doesn't allow us to +# override the VPATH value unless we set PROJECT_NAME, which we don't want +# to do. +VPATH = $(LLVM_SRC_ROOT)/utils/unittest/googletest/src/ Why play with VPATH here? What is this doing? Can this be handled a different way? Similarly in makefile.unittest. +++ unittests/ADT/DenseMapTest.cpp (revision 0) +namespace { + +using namespace llvm; Please put the 'using' right after the #includes. Otherwise, the patch looks good to me. Please verify that it works with both a SRCDIR=OBJDIR and SRCDIR!=OBJDIR build. -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081231/375a8f4d/attachment.html>
Chris Lattner wrote:> On Dec 30, 2008, at 2:54 PM, Talin wrote: >> OK changes made and new patch attached. > > Nice > > +++ utils/unittest/Makefile (revision 0) > > ... > +# This has to come after Makefile.common, since it doesn't allow us to > +# override the VPATH value unless we set PROJECT_NAME, which we don't > want > +# to do. > +VPATH = $(LLVM_SRC_ROOT)/utils/unittest/googletest/src/ > > > Why play with VPATH here? What is this doing? Can this be handled a > different way? Similarly in makefile.unittest.This may be the result of my inability to completely figure out how to do certain things within the LLVM makefile framework. Specifically, I was trying to leave the googletest directory untouched, and since there's already a Makefile in that dir (which won't work for our purposes), I needed to build the gtest library using the makefile two directory levels up (in llvm/utils/unittest). Similarly, in the case of the Makefile.unittest, I was trying to include TestMain.cpp in the builds for all the subdirs. In order to work with SRCDIR != OBJDIR, the files in SOURCES need to have relative paths so that the .o and .d files go in the right places in OBJDIR; But in order to find the sources, we have to give $wildcard an absolute path to SRCDIR. I see that in some places in LLVM, this is accomplished by stripping off the directory portion entirely from the output of $wildcard, however that only works for source files that are in the same directory as the makefile. So messing with VPATH allowed me to use relative paths everywhere. I couldn't figure out how to do it without VPATH.> > +++ unittests/ADT/DenseMapTest.cpp (revision 0) > +namespace { > + > +using namespace llvm; > > > Please put the 'using' right after the #includes. > > Otherwise, the patch looks good to me. Please verify that it works > with both a SRCDIR=OBJDIR and SRCDIR!=OBJDIR build. > > -Chris > ------------------------------------------------------------------------ > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >
2008/12/31 Chris Lattner <clattner at apple.com>> On Dec 30, 2008, at 2:54 PM, Talin wrote: > > OK changes made and new patch attached. > > +++ utils/unittest/Makefile (revision 0) > > ... > +# This has to come after Makefile.common, since it doesn't allow us to > +# override the VPATH value unless we set PROJECT_NAME, which we don't want > +# to do. > +VPATH = $(LLVM_SRC_ROOT)/utils/unittest/googletest/src/ > > > Why play with VPATH here? What is this doing? Can this be handled a > different way? Similarly in makefile.unittest. >Removed VPATH in this file by moving GTest up one directory, and added VPATH-less Makefiles: http://llvm.org/viewvc/llvm-project?view=rev&revision=61539 http://llvm.org/viewvc/llvm-project?view=rev&revision=61540 (minor revision) Also removed VPATH from Makefile.unittest (see link below). +++ unittests/ADT/DenseMapTest.cpp (revision 0)> +namespace { > + > +using namespace llvm; > > > Please put the 'using' right after the #includes. >Done.> Otherwise, the patch looks good to me. Please verify that it works with > both a SRCDIR=OBJDIR and SRCDIR!=OBJDIR build. >Done for both; passes both with flying colors. Submitted: http://llvm.org/viewvc/llvm-project?view=rev&revision=61541 Thanks to Talin for the original patch. Now everyone can go write some unittests! Misha P.S. Why are you still reading? Go! Write! Test! -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081231/d90717b7/attachment.html>
Hooray! I'm thinking that getting unit tests for the classes in ADT should be an early goal. I also wanted to mention a point about the general philosophy of unit testing, which is that the presence of such tests alters the calculation of risk when making changes to a code base. Programmers have various rules of thumb for estimating risk - for example, a change which affects a large number of lines of code is generally perceived to be riskier than a change which affects a small number of lines. But at the same time, some very large changes can carry almost no risk - such as changing the name of a symbol which occurs in thousands of places in the code - because the compiler will tell you if you made a mistake anywhere. What unit tests do is allow that same kind of assurance at the semantic level that the compiler's syntax checking gives you at the syntactical level. And since reduction of risk in one area allows taking greater risks elsewhere, it can act as an enabler for global refactorings and other radical improvements that would have been perceived as too risky otherwise. This ability of unit tests to reduce perceived risk works best if the tests are comprehensive, testing even trivial aspects of the target class (such as brand new container correctly reporting empty()). -- Talin Misha Brukman wrote:> 2008/12/31 Chris Lattner <clattner at apple.com> > > >> On Dec 30, 2008, at 2:54 PM, Talin wrote: >> >> OK changes made and new patch attached. >> >> +++ utils/unittest/Makefile (revision 0) >> >> ... >> +# This has to come after Makefile.common, since it doesn't allow us to >> +# override the VPATH value unless we set PROJECT_NAME, which we don't want >> +# to do. >> +VPATH = $(LLVM_SRC_ROOT)/utils/unittest/googletest/src/ >> >> >> Why play with VPATH here? What is this doing? Can this be handled a >> different way? Similarly in makefile.unittest. >> >> > > Removed VPATH in this file by moving GTest up one directory, and added > VPATH-less Makefiles: > http://llvm.org/viewvc/llvm-project?view=rev&revision=61539 > http://llvm.org/viewvc/llvm-project?view=rev&revision=61540 (minor revision) > > Also removed VPATH from Makefile.unittest (see link below). > > +++ unittests/ADT/DenseMapTest.cpp (revision 0) > >> +namespace { >> + >> +using namespace llvm; >> >> >> Please put the 'using' right after the #includes. >> >> > > Done. > > > >> Otherwise, the patch looks good to me. Please verify that it works with >> both a SRCDIR=OBJDIR and SRCDIR!=OBJDIR build. >> >> > > Done for both; passes both with flying colors. > Submitted: http://llvm.org/viewvc/llvm-project?view=rev&revision=61541 > > Thanks to Talin for the original patch. > > Now everyone can go write some unittests! > > Misha > > P.S. Why are you still reading? Go! Write! Test! > > > ------------------------------------------------------------------------ > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >