[PATCH] D68135: [lit] Set the target-windows feature for any windows environment

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In D68135#1694884 <https://reviews.llvm.org/D68135#1694884>, @mstorsjo wrote:

> In D68135#1694738 <https://reviews.llvm.org/D68135#1694738>, @probinson wrote:
>
> > In D68135#1694106 <https://reviews.llvm.org/D68135#1694106>, @mstorsjo wrote:
> >
> > > Updated, with a slightly different form of the regex, that also allows the triple to just end at -windows.
> > >
> > > Alternatively, we could just remove this part of the lit config altogether. There's no other OSes that have target-<os> as a feature (only system-<os> for the host where the test is running), and after D68133 <https://reviews.llvm.org/D68133> and D68136 <https://reviews.llvm.org/D68136>, no tests actually use this feature any longer.
> >
> >
> > In most cases, target restrictions rely on using a component of the triple.  If all Windows-target triples are guaranteed to spell the component 'windows' then `REQUIRES: windows` works, and we don't need target-windows at all.  But I was under the impression that `win32` was used in the triple sometimes?
>
>
> At least at this point, it seems to use normalized triple names. For input to tools, win32 is a synonym to windows, and "mingw32" as OS name gets normalized to "windows-gnu".


Right, D47381 <https://reviews.llvm.org/D47381> / rL339307 <https://reviews.llvm.org/rL339307> replaced all uses of REQUIRES: win32.

I'm going to assume that target-windows was added to deal with the common case of wanting to XFAIL a test for mingw and windows at the same time. Now that it's easy to match them as just XFAIL: windows or REQUIRES: windows, I don't think we really need this target-windows feature.

As you mention, there are two remaining tests using target-windows. If you want to update this patch to delete the target-windows feature and migrate them to just "windows", that works for me. Less code is always better.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68135/new/

https://reviews.llvm.org/D68135