jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land.
LGTM, but might not hurt to have someone else looking at it. ================ Comment at: llvm/test/FileCheck/comment/bad-comment-prefix.txt:40 +RUN: -check-prefixes=RUN,FOO | \ +RUN: FileCheck -check-prefix=CHECK-PREFIX-DUP-RUN_ %s +CHECK-PREFIX-DUP-COM: error: supplied check prefix must be unique among check and comment prefixes: 'COM' ---------------- I'm guessing the underscore is to circumvent lit trying to run things? If so, I think we need to fix lit to only run lines where the RUN: is at the start of the line (ignoring whitespace and non alpha-numerics). I don't think anything else makes sense. Not related to this patch of course, so nothing to do here, unless my guess is incorrect. ================ Comment at: llvm/test/FileCheck/comment/suffixes.txt:1-2 +# Comment prefixes plus check directive suffixes are not comment directives +# and are treated as plain text. + ---------------- jdenny wrote: > jhenderson wrote: > > I don't think you should change anything here, but if I'm following this > > right, this leads to the amusing limitation that you can "comment out" (via > > --comment-prefixes) any CHECK lines that use a suffix (CHECK-NEXT, > > CHECK-NOT etc) but not those that don't without changing --check-prefixes > > value! > > > > By the way, do we need to have a test-case for that? I.e. that > > --comment-prefixes=CHECK-NEXT disables the CHECK-NEXT lines (assuming it > > does of course)? > > I don't think you should change anything here, but if I'm following this > > right, this leads to the amusing limitation that you can "comment out" (via > > --comment-prefixes) any CHECK lines that use a suffix (CHECK-NEXT, > > CHECK-NOT etc) but not those that don't without changing --check-prefixes > > value! > > That's right, but check prefixes have this problem too. That is, you can do > things like `-check-prefixes=FOO,FOO-NOT` so that `FOO-NOT` is not negative. > > `ValidatePrefixes` should be extended to catch such cases, I think. But in a > separate patch. > > > By the way, do we need to have a test-case for that? I.e. that > > --comment-prefixes=CHECK-NEXT disables the CHECK-NEXT lines (assuming it > > does of course)? > > Hmm. I think it's behavior we don't want to support. Maybe the test case > should be added when extending `ValidatePrefixes` as I described above? > I agree it's separate work. FWIW, I just came up with a genuinely useful use-case for it with CHECK directives, but it might just be unnecessary. Imagine the case where you want a test where some specific output is printed under one condition and not another condition. You'd want something like: ``` # RUN: ... | FileCheck %s --check-prefix=WITH # RUN: ... | FileCheck %s --check-prefix=WITHOUT # WITH: some text that should be matched # WITHOUT-NOT: some text that should be matched ``` A careleses developer could change the text of "WITH" to match some new behaviour without changing "WITHOUT-NOT", thus breaking the second case. You could instead do: ``` # RUN: ... | FileCheck %s --check-prefix=CHECK-NOT # RUN: ... | FileCheck %s # CHECK-NOT: some text that should be matched ``` Then, if the output changed, you'd update both the regular and NOT match. I might have used this pattern in a few tests in the past had it occurred to me. Anyway, I think there are other ways of solving that problem, although not without work on FileCheck (I've been prototyping a method with only limited success so far), and I agree it's otherwise mostly horrible, so I'm not seriously opposing the suggestion. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79276/new/ https://reviews.llvm.org/D79276 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
