llvm dev - May 2019 - RFC: "REQUIRES: no*" considered harmful

Dear test fans,

Recently a couple of cases came to light, where a test had the line
    REQUIRES: nowindows
with the obvious expectation that it would be disabled, but only for
the Windows platform.  Sadly, this is not true; the "no" prefix means
nothing to Lit.  I've already fixed one test, within Lit's own test 
suite, and I've done a bunch of grepping to see what else is lurking.

The various config scripts define feature keywords with "no" or
"not_"
or "non-" prefixes in only a handful of cases.  And really these extra
keywords are unnecessary, because we can disable a test using the positive 
form with UNSUPPORTED:.  And, I claim (with the obvious evidence of two
tests that fell into this trap) that having negative keywords for some
cases can mislead people and end up having entirely the wrong effect.

I propose to eliminate the negative forms of the feature keywords and
modify all the affected tests to use UNSUPPORTED instead. Well actually
I'd modify the affected tests first, but you get the idea.

There are 7 feature keywords that start with "no" or "not_"
or "non-"
that I can find, and it's trivial to have only positive versions
(and in fact the last 4 already have positive versions):
    non-ms-sdk  (clang only)
    non-ps4-sdk (clang only)
    not_COFF    (llvm only)
    not_asan
    not_msan
    not_ubsan
    nozlib

Very few in-tree tests are affected, and I'm happy to do that part.
I'm finding 9 tests in clang, 6 tests in LLVM, and 1 test in LLDB
that match the regex 'REQUIRES:.* no'.  (The LLDB test is the other
victim of "nowindows" that I've found.)

If you believe this is a BAD idea, please speak up and explain why.
In the meantime I'll start putting together a patch set for this.

Thanks,
--paulr

Good idea to remove these. Also because of their inconsistent spelling.

Michael

Am Do., 9. Mai 2019 um 13:37 Uhr schrieb via llvm-dev <llvm-dev at
lists.llvm.org>:
>
> Dear test fans,
>
> Recently a couple of cases came to light, where a test had the line
>     REQUIRES: nowindows
> with the obvious expectation that it would be disabled, but only for
> the Windows platform.  Sadly, this is not true; the "no" prefix
means
> nothing to Lit.  I've already fixed one test, within Lit's own test
> suite, and I've done a bunch of grepping to see what else is lurking.
>
> The various config scripts define feature keywords with "no" or
"not_"
> or "non-" prefixes in only a handful of cases.  And really these
extra
> keywords are unnecessary, because we can disable a test using the positive
> form with UNSUPPORTED:.  And, I claim (with the obvious evidence of two
> tests that fell into this trap) that having negative keywords for some
> cases can mislead people and end up having entirely the wrong effect.
>
> I propose to eliminate the negative forms of the feature keywords and
> modify all the affected tests to use UNSUPPORTED instead. Well actually
> I'd modify the affected tests first, but you get the idea.
>
> There are 7 feature keywords that start with "no" or
"not_" or "non-"
> that I can find, and it's trivial to have only positive versions
> (and in fact the last 4 already have positive versions):
>     non-ms-sdk  (clang only)
>     non-ps4-sdk (clang only)
>     not_COFF    (llvm only)
>     not_asan
>     not_msan
>     not_ubsan
>     nozlib
>
> Very few in-tree tests are affected, and I'm happy to do that part.
> I'm finding 9 tests in clang, 6 tests in LLVM, and 1 test in LLDB
> that match the regex 'REQUIRES:.* no'.  (The LLDB test is the other
> victim of "nowindows" that I've found.)
>
> If you believe this is a BAD idea, please speak up and explain why.
> In the meantime I'll start putting together a patch set for this.
>
> Thanks,
> --paulr
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

> -----Original Message-----
> From: Michael Kruse [mailto:llvmdev at meinersbur.de]
> Sent: Wednesday, May 22, 2019 3:37 PM
> To: Robinson, Paul
> Cc: llvm-dev
> Subject: Re: [llvm-dev] RFC: "REQUIRES: no*" considered harmful
> 
> Good idea to remove these. Also because of their inconsistent spelling.
Thanks for the +1.  I should have noted here, these 'lit' attributes
were removed in r360603.
--paulr
> 
> Michael
> 
> Am Do., 9. Mai 2019 um 13:37 Uhr schrieb via llvm-dev <llvm-
> dev at lists.llvm.org>:
> >
> > Dear test fans,
> >
> > Recently a couple of cases came to light, where a test had the line
> >     REQUIRES: nowindows
> > with the obvious expectation that it would be disabled, but only for
> > the Windows platform.  Sadly, this is not true; the "no"
prefix means
> > nothing to Lit.  I've already fixed one test, within Lit's own
test
> > suite, and I've done a bunch of grepping to see what else is
lurking.
> >
> > The various config scripts define feature keywords with "no"
or "not_"
> > or "non-" prefixes in only a handful of cases.  And really
these extra
> > keywords are unnecessary, because we can disable a test using the
> positive
> > form with UNSUPPORTED:.  And, I claim (with the obvious evidence of
two
> > tests that fell into this trap) that having negative keywords for some
> > cases can mislead people and end up having entirely the wrong effect.
> >
> > I propose to eliminate the negative forms of the feature keywords and
> > modify all the affected tests to use UNSUPPORTED instead. Well
actually
> > I'd modify the affected tests first, but you get the idea.
> >
> > There are 7 feature keywords that start with "no" or
"not_" or "non-"
> > that I can find, and it's trivial to have only positive versions
> > (and in fact the last 4 already have positive versions):
> >     non-ms-sdk  (clang only)
> >     non-ps4-sdk (clang only)
> >     not_COFF    (llvm only)
> >     not_asan
> >     not_msan
> >     not_ubsan
> >     nozlib
> >
> > Very few in-tree tests are affected, and I'm happy to do that
part.
> > I'm finding 9 tests in clang, 6 tests in LLVM, and 1 test in LLDB
> > that match the regex 'REQUIRES:.* no'.  (The LLDB test is the
other
> > victim of "nowindows" that I've found.)
> >
> > If you believe this is a BAD idea, please speak up and explain why.
> > In the meantime I'll start putting together a patch set for this.
> >
> > Thanks,
> > --paulr
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev