[PATCH] D87830: [clang-tidy][test] Allow empty checks in check_clang_tidy.py

[Endre Fülöp via Phabricator via cfe-commits]

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