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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1980-1981
+                                  SMLoc::getFromPointer(E.getVarName().data()));
+        // SM.GetMessage(SMLoc::getFromPointer(E.getVarName.data()),
+        // SourceMgr::DK_Error, ErrMsg)
+      });
----------------
Why is there commented out code here?


================
Comment at: llvm/lib/Support/FileCheck.cpp:2230-2236
+      Error MatchErrors = getMatchErrors(MatchResult.takeError());
+      if (MatchErrors) {
+        Pat->printMatchErrors(SM, Buffer, std::move(MatchErrors), Diags);
+        DirectiveFail = true;
+      } else
+        printNoMatch(/*ExpectedMatch=*/false, SM, Prefix, Pat->getLoc(), *Pat,
+                     1, Buffer, Req.VerboseVerbose, Diags);
----------------
Aside from the `DirectiveFail = true` line here, this and the other two instances of this pattern look identical to me.   I guess that's partly my fault for the suggestion earlier of not returning a boolean to indicate an error. Perhaps part of the problem previously was the suggestion that the `printNoMatch` function even returned anything. I certainly wouldn't expect it.

How about we wrap this common code in a helper function, called `handleMatchFailure` or something like that, and that can return a boolean saying whether the failure was due to an error or regular failure? We could invent an enum that indicates the same thing but with better words, to avoid the boolean, but that might be going further than necessary. An alternative would be to pass in a lambda `OnMatchError` or something, which does nothing in the two other cases, but here sets the `DirectiveFail` variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86222