gamesh411 added a comment. In D87830#2298198 <https://reviews.llvm.org/D87830#2298198>, @aaron.ballman wrote:
> > know of any tests that are impacted by this? I haven't found any tidy-tests that were negative-tests (ie.: tests that assert that there are no diagnostics). > ... if I understand the proposed patch, that may now be silently accepted as > a passing test? This is not entirely the case. Let me demonstrate with a few cases: Before this patch: // empty test // RUN: %check_clang_tidy %s %t // no CHECK lines in the entire file In the above case the test will fail with `CHECK-FIXES, CHECK-MESSAGES or CHECK-NOTES not found in the input`. // empty test, but explicityly mention we dont want diagnostics // RUN: %check_clang_tidy %s %t // CHECK_MESSAGES_NOT: ARBITRARY STRING HERE // ... // there are *no* other CHECK messages This passes the first round of sanity checking, but FileCheck nevertheless rejects the processed test file: FileCheck error: '/home/gamesh411/clang-rwa/tools/clang/tools/extra/test/clang-tidy/checkers/Output/empty.cpp.tmp.cpp.msg' is empty. // empty test, but explicityly mention we dont want diagnostics // RUN: %check_clang_tidy %s %t // CHECK_MESSAGES_NOT: arbitrary string here, I prefer NO DIAG // ... // there *are* some other CHECK messages further down This is fine, but only means that the arbitrary string diagnostic is expected to be *not* seen, while some others are. After patch: The second case above becomes fine, everything else stays the same. > Or should this not be an issue in general because `// CHECK` lines should > still be present in the test and those will fail when compared against an > empty file? This means that the only accidental empty test scenario is when someone accidentally skips the CHECK messages he wants to put in, *but* manages to put in the CHECK-NOT by accident as well. This can result from copy-paste, but still, I think this is not likely. > I'm wondering if we want to require something like `// > expected-no-diagnostics` from the `-verify` command in Clang so that users > have to explicitly opt a test into the behavior? My thoughts were exactly that; I need something like the DiagVerifier's `expected-no-diagnostics`. That was the motivation behind this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87830/new/ https://reviews.llvm.org/D87830 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
