[PATCH] D86222: Fix PR46880: Fail CHECK-NOT with undefined variable

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre marked an inline comment as done.
thopre added a comment.

In D86222#2251434 <https://reviews.llvm.org/D86222#2251434>, @jhenderson wrote:

> Code mostly looks fine now, but there are still plenty of test failures being reported by the pre-merge bot. What's the situation with those currently?

I've started trying to fix them but sadly most of them are non trivial which is even more difficult when one is not familiar with what the test is trying to check. It seems a fair amount are using several CHECK-NOT as a way to say "I don't want THIS followed by THAT to match the input", where THAT uses a variable defined in THIS. It basically expects a back reference. This can be fixed by merging the two CHECK-NOT when they don't use different prefix but it seems to be yet another example where a grouping directive would be useful, e.g.:

CHECK-REGION-START: REGION-A
CHECK: THIS
CHECK: THAT
CHECK-REGION-END: REGION-A
CHECK-NOT: REGION-A

This is just an example, better syntax would need to be use. The reason I suggest a region rather than a new special-purpose directive is I wanted to have something like REGION-DAG where directives in a region are CHECK: that need to appear linearly but directives in different regions can be interleaved. Anyway, my plan is to try to merge those CHECK-NOT directives and then we can discuss how to introduce new syntax to make this more readable.



================
Comment at: llvm/lib/Support/FileCheck.cpp:2014
+  // the next line.
+  Buffer = Buffer.substr(Buffer.find_first_not_of(" \t\n\r"));
+  FileCheckDiag::MatchType MatchTy = FileCheckDiag::MatchError;
----------------
jhenderson wrote:
> This might be a dumb question, but I'm not following why we're skipping '\t' and ' ' here. It certainly doesn't line up with the comment above.
I'm not sure, this is copied over from PrintNoMatch. I thought it is used so that the "scanning from here" shows the first non blank line after last match but I haven't looked in details.


================
Comment at: llvm/lib/Support/FileCheck.cpp:2031-2046
+  // Print undefined variable diagnostics.
+  MatchErrors =
+      handleErrors(std::move(MatchErrors), [&](const UndefVarError &E) {
+        SmallString<256> Msg;
+        raw_svector_ostream OS(Msg);
+        OS << "uses undefined variable: ";
+        E.log(OS);
----------------
jhenderson wrote:
> Can't these two be folded into a single `handleAllErrors` call? IIRC, `handleAllErrors` takes any number of handler lambdas, and iterates through them until one matches, or it runs out. Thus the above code should look like the something like the suggested edit.
It can be merged yes. It's a relic from when I would group all undefined variables in one error message in which case the undefined variable errors needs to be processed together. I'll change that once I'm back to work on Monday.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86222/new/

https://reviews.llvm.org/D86222