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
Michael Kruse via llvm-dev
2019-May-22 19:36 UTC
[llvm-dev] RFC: "REQUIRES: no*" considered harmful
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 > 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 > > lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Seemingly Similar Threads
- SelectionDAGISel::Select's API considered harmful
- "wbinfo -u" considered harmful towards Winbindd...
- SelectionDAGISel::Select's API considered harmful
- SelectionDAGISel::Select's API considered harmful
- [LLVMdev] Extending GetElementPointer, or Premature Linearization Considered Harmful