+++ unittests/TestMain.cpp (revision 0) +//===--- unittest.cpp - unittest driver -----------------------------------===// +++ unittests/ADT/DenseMapTest.cpp (revision 0) +//===- llvm/unittest/DenseMapMap.h - DenseMap unit tests --------*- C++ -*-===// You probably want to update these file headers to match their real names. 2008/12/30 Misha Brukman <brukman at gmail.com>> 2008/12/30 Talin <viridia at gmail.com> > >> Here's the version of the unit test patch, incorporating the feedback I >> have received so far. >> > > Looks good, a few comments below. > > >> Some notes on the patch: >> >> - This patch doesn't include googletest itself, that will need to be >> checked in separately. The source distribution will live in >> llvm/utils/unittest/googletest. (The reason for the extra directory level is >> so that the LLVM-specific makefiles can live in llvm/utils/unittest, while >> the googletest directory itself can be the pristine, unmodified >> distribution.) >> >> Are you going to send a separate patch for this, or should I do this? > >> >> - >> - The individual tests are in llvm/unittests (note plural). >> - The makefile target to build and run the tests is "make unittest" (I >> wasn't sure if you could have a synthetic target that was the same as the >> name of a directory, so I made it "unittest" instead of "unittests") >> >> I don't think that's an issue -- to recurse into a directory, you have to > say "make -C dir [target]". > >> >> - >> - There's a common Makefile and a common TestMain.cpp in >> llvm/unittests. The individual tests are in llvm/unittests/<dir> where <dir> >> is the name of an LLVM library such as "ADT". Each of these subdirs has a >> tiny makefile which sets TESTNAME=<name> and then includes the common >> Makefile.unittest. Each subdir under unittests creates a separate >> executable. (I didn't use the default googletest main because I figured at >> some point we might want to customize main().) >> - I updated the LICENSE.txt and mkpatch, but I haven't done the HTML >> docs yet because I am still thinking about what to write. >> - I probably made some mistakes in setting up the makefile rules - >> that is what took the most time - so it will merit heightened scrutiny. >> >> > +++ unittests/Makefile.unittest (revision 0) > [...] > +VPATH=$(PROJ_SRC_DIR) $(PROJ_SRC_ROOT)/unittests > > That doesn't look like a valid path to me. > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081230/d1e9056d/attachment.html>
OK changes made and new patch attached. Comments below: On Tue, Dec 30, 2008 at 2:40 PM, Misha Brukman <brukman at gmail.com> wrote:> +++ unittests/TestMain.cpp (revision 0) > +//===--- unittest.cpp - unittest driver > -----------------------------------===// > > +++ unittests/ADT/DenseMapTest.cpp (revision 0) > +//===- llvm/unittest/DenseMapMap.h - DenseMap unit tests --------*- C++ > -*-===// > > You probably want to update these file headers to match their real names. > > 2008/12/30 Misha Brukman <brukman at gmail.com> > >> 2008/12/30 Talin <viridia at gmail.com> >> >>> Here's the version of the unit test patch, incorporating the feedback I >>> have received so far. >>> >> >> Looks good, a few comments below. >> >> >>> Some notes on the patch: >>> >>> - This patch doesn't include googletest itself, that will need to be >>> checked in separately. The source distribution will live in >>> llvm/utils/unittest/googletest. (The reason for the extra directory level is >>> so that the LLVM-specific makefiles can live in llvm/utils/unittest, while >>> the googletest directory itself can be the pristine, unmodified >>> distribution.) >>> >>> Are you going to send a separate patch for this, or should I do this? >> >Please go ahead and do this. It seems silly (to me anyway) to convert the source tarball into a patch file.> >>> - >>> - The individual tests are in llvm/unittests (note plural). >>> - The makefile target to build and run the tests is "make unittest" >>> (I wasn't sure if you could have a synthetic target that was the same as the >>> name of a directory, so I made it "unittest" instead of "unittests") >>> >>> I don't think that's an issue -- to recurse into a directory, you have to >> say "make -C dir [target]". >> >>> >>> - >>> - There's a common Makefile and a common TestMain.cpp in >>> llvm/unittests. The individual tests are in llvm/unittests/<dir> where <dir> >>> is the name of an LLVM library such as "ADT". Each of these subdirs has a >>> tiny makefile which sets TESTNAME=<name> and then includes the common >>> Makefile.unittest. Each subdir under unittests creates a separate >>> executable. (I didn't use the default googletest main because I figured at >>> some point we might want to customize main().) >>> - I updated the LICENSE.txt and mkpatch, but I haven't done the HTML >>> docs yet because I am still thinking about what to write. >>> - I probably made some mistakes in setting up the makefile rules - >>> that is what took the most time - so it will merit heightened scrutiny. >>> >>> >> +++ unittests/Makefile.unittest (revision 0) >> [...] >> +VPATH=$(PROJ_SRC_DIR) $(PROJ_SRC_ROOT)/unittests >> >That's two dirs: One for the current directory, and one for the directory one level above. ".." won't work here, so I have to create an absolute path so that it can find TestMain.cpp.> >> >> That doesn't look like a valid path to me. >> >> > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >-- -- Talin -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081230/cdbef21e/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: unittest.patch Type: application/octet-stream Size: 12295 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081230/cdbef21e/attachment.obj>
2008/12/30 Talin <viridia at gmail.com>> OK changes made and new patch attached. >I'm not sure how I missed this the first time, or if it's new: +++ unittests/TestMain.cpp (revision 0) +//===--- TestMail.cpp - unittest driver -----------------------------------===// Same comment as previously. :-)> On Tue, Dec 30, 2008 at 2:40 PM, Misha Brukman <brukman at gmail.com> wrote: > >> 2008/12/30 Misha Brukman <brukman at gmail.com> >> >>> 2008/12/30 Talin <viridia at gmail.com> >>> >>>> Some notes on the patch: >>>> >>>> - This patch doesn't include googletest itself, that will need to be >>>> checked in separately. The source distribution will live in >>>> llvm/utils/unittest/googletest. (The reason for the extra directory level is >>>> so that the LLVM-specific makefiles can live in llvm/utils/unittest, while >>>> the googletest directory itself can be the pristine, unmodified >>>> distribution.) >>>> >>>> Are you going to send a separate patch for this, or should I do this? >>> >> > Please go ahead and do this. It seems silly (to me anyway) to convert the > source tarball into a patch file. >Yes, I figured this out after trying to see what's involved there. I'm thinking we don't need to keep everything in the tarball, because it'll just bloat the LLVM distribution. I think we should be fine with just the include/ and src/ directories, and I'll keep the COPYING file since it's the license, and add a README.LLVM file. Sound good?> >>>> - >>>> - The individual tests are in llvm/unittests (note plural). >>>> - The makefile target to build and run the tests is "make unittest" >>>> (I wasn't sure if you could have a synthetic target that was the same as the >>>> name of a directory, so I made it "unittest" instead of "unittests") >>>> >>>> I don't think that's an issue -- to recurse into a directory, you have >>> to say "make -C dir [target]". >>> >>So can we call the target to build and run unittests "unittests" or does it create problems? -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081230/0abb6108/attachment.html>
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>