I've modified this test so it doesn't write to the source tree (by adding -o /dev/null) in r210618, but I'll echo Ried and Eric's sentiments and add one of my own:
"test this command doesn't crash" is a fairly poor test case. Please verify that whatever behavior you expect (and I'm sure it's something more precise than "does anything other than crash") actually occurs (using FileCheck, presumably). Crashes are a great way to find missing test coverage, and simply adding a "this doesn't crash" test is a real lost opportunity to actually test whatever we had failed to test previously. And, yes, as Eric/Reid said, if you've found missing test coverage in other areas, add that to the relevant test directories. The driver should be tested in test/Driver. On Tue, Jun 10, 2014 at 4:58 PM, Bob Wilson <[email protected]> wrote: > It would be sufficient, but since I think any future breakage of this is > equally likely to come from the driver, I intentionally made the test to run > through the driver. > > On Jun 10, 2014, at 3:09 PM, Reid Kleckner <[email protected]> wrote: > > This runline is sufficient, then: > // RUN: %clang_cc1 %s -triple x86_64-pc-win32-macho -emit-llvm-only > > > On Tue, Jun 10, 2014 at 2:46 PM, Bob Wilson <[email protected]> wrote: >> >> The most important check is for header search, but I was hoping it might >> serve as an overall sanity check for other issues. I already found one issue >> with i386 that is not exposed with -fsyntax-only. (I’m looking at that now >> and will fix it if there’s an easy solution.) I don’t feel strongly about >> this test, so in the interest of reducing testing time, I’ll change it to >> use -fsyntax-only. >> >> > On Jun 10, 2014, at 2:29 PM, Eric Christopher <[email protected]> >> > wrote: >> > >> > What are you trying to test with this test? It's currently invoking >> > the backend and I'm not sure I see a reason for it given the original >> > change is only to header search? >> > >> > -eric >> > >> > On Tue, Jun 10, 2014 at 2:07 PM, Bob Wilson <[email protected]> >> > wrote: >> >> Author: bwilson >> >> Date: Tue Jun 10 16:07:12 2014 >> >> New Revision: 210584 >> >> >> >> URL: http://llvm.org/viewvc/llvm-project?rev=210584&view=rev >> >> Log: >> >> Fix crash with x86_64-pc-win32-macho target. <rdar://problem/17235840> >> >> >> >> The changes in r204978 broke win32-macho targets. There were checks >> >> added for >> >> MSVC and Itanium environments as special cases, and win32-macho needs >> >> to be >> >> treated the same way. >> >> >> >> Added: >> >> cfe/trunk/test/Misc/win32-macho.c >> >> Modified: >> >> cfe/trunk/lib/Frontend/InitHeaderSearch.cpp >> >> >> >> Modified: cfe/trunk/lib/Frontend/InitHeaderSearch.cpp >> >> URL: >> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/InitHeaderSearch.cpp?rev=210584&r1=210583&r2=210584&view=diff >> >> >> >> ============================================================================== >> >> --- cfe/trunk/lib/Frontend/InitHeaderSearch.cpp (original) >> >> +++ cfe/trunk/lib/Frontend/InitHeaderSearch.cpp Tue Jun 10 16:07:12 >> >> 2014 >> >> @@ -472,7 +472,8 @@ void InitHeaderSearch::AddDefaultInclude >> >> >> >> case llvm::Triple::Win32: >> >> if (triple.getEnvironment() == llvm::Triple::MSVC || >> >> - triple.getEnvironment() == llvm::Triple::Itanium) >> >> + triple.getEnvironment() == llvm::Triple::Itanium || >> >> + triple.getObjectFormat() == llvm::Triple::MachO) >> >> return; >> >> break; >> >> } >> >> >> >> Added: cfe/trunk/test/Misc/win32-macho.c >> >> URL: >> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/win32-macho.c?rev=210584&view=auto >> >> >> >> ============================================================================== >> >> --- cfe/trunk/test/Misc/win32-macho.c (added) >> >> +++ cfe/trunk/test/Misc/win32-macho.c Tue Jun 10 16:07:12 2014 >> >> @@ -0,0 +1,2 @@ >> >> +// Check that basic use of win32-macho targets works. >> >> +// RUN: %clang -c -target x86_64-pc-win32-macho %s >> >> >> >> >> >> _______________________________________________ >> >> cfe-commits mailing list >> >> [email protected] >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
