jhenderson added inline comments.
================ 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: > > 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. > I agree the use case is important, and I also agree there must be a better > solution. > > The underlying issue is that we want to reuse a pattern. Perhaps there > should be some way to define a FileCheck variable once and reuse it among > multiple FileCheck commands. For example: > > ``` > # RUN: ... | FileCheck %s --check-prefix=WITH,ALL > # RUN: ... | FileCheck %s --check-prefix=WITHOUT,ALL > > # ALL-DEF: [[VAR:some text that should be matched]] > # WITH: [[VAR]] > # WITHOUT-NOT: [[VAR]] > ``` > > It should probably be possible to use regexes in such a variable. I'm not > sure if that's possible now. It might require a special variable type. We > currently have `#` to indicate numeric variables. Perhaps `~` would indicate > pattern variables. That's almost exactly what I've been prototyping on-and-off over the past few months, but I've been running into various ordering issues, which I haven't yet solved to my satisfaction. My original thread that inspired the idea is http://lists.llvm.org/pipermail/llvm-dev/2020-January/138822.html. 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
