jdenny marked an inline comment as done. jdenny 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. + ---------------- jhenderson wrote: > 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. Cool. I think I could benefit from this feature. Repository: rG LLVM Github Monorepo 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
