jdenny marked 5 inline comments as done. jdenny added a comment. In D79276#2024411 <https://reviews.llvm.org/D79276#2024411>, @jhenderson wrote:
> LGTM, but might not hurt to have someone else looking at it. I'll probably wait until Monday to give others more time to participate. Thanks for the review! ================ 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' ---------------- jhenderson wrote: > 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. > I'm guessing the underscore is to circumvent lit trying to run things? Yep. > 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. I agree lit should change. That might be the right condition. People do things like the following, but maybe they shouldn't: ``` clang/test/Index/complete-access-checks-crash.cpp: Derived(). // RUN: c-index-test -code-completion-at=%s:10:15 %s | FileCheck %s ``` At the very least, lit shouldn't recognize `RUN:` at the ends of other words (where `-` is considered a word character). Lit even recognizes `XRUN:`. ================ 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: > > > 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. 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
