Thanks for fixing the output to the source tree. I should have caught that.
I don’t especially know what to test for here. I wasn’t involved in setting up this win32-macho stuff, and I don’t know much about how it works. Apparently whoever did set it up neglected to add any tests. I won’t claim this is a good test, but it is far better than what we had before (nothing!). If someone who knows about this can provide a better test, then that would be great. On Jun 10, 2014, at 6:00 PM, David Blaikie <[email protected]> wrote: > 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
